Opened 6 years ago

Closed 3 years ago

#16421 closed defect (patchwelcome)

specifying string value in staticHasFeatures non-intuitive

Reported by: bill Owned by: Rawld Gill
Priority: undecided Milestone: 1.13
Component: BuildSystem Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description

See attached patch which adds

"css-user-select": "WebKitUserSelect"

to the staticHasFeatures list.

The source line:

var cssUserSelect = has("css-user-select");

gets converted to:

var cssUserSelect =  WebKitUserSelect ;

Where'd the quotes go?

The webkitMobile build profile specifies another string:

"config-selectorEngine": "lite",

which is producing no obvious build error.

Attachments (1)

webkitMobileProfile.patch (877 bytes) - added by bill 6 years ago.
attempt to add string

Download all attachments as: .zip

Change History (13)

Changed 6 years ago by bill

Attachment: webkitMobileProfile.patch added

attempt to add string

comment:1 Changed 6 years ago by bill

Milestone: tbd1.8.2
Owner: changed from Rawld Gill to bill
Status: newassigned

comment:2 Changed 6 years ago by bill

Resolution: fixed
Status: assignedclosed

In [30108]:

Fix builder's handling of strings in staticHasFeatures, and add new "css-user-select" has() flag from [30098] to webkitMobile build, fixes #16421 and refs #15990 on trunk !strict

comment:15 Changed 6 years ago by bill

In [30109]:

Fix builder's handling of strings in staticHasFeatures, and add new "css-user-select" has() flag from [30098] to webkitMobile build, fixes #16421 and refs #15990 on 1.8 branch !strict

comment:16 Changed 6 years ago by Colin Snover

Resolution: fixed
Status: closedreopened

It’s not true that you can’t specify a string. As shown in the comments in the code, you provide a double-quoted string. This patch breaks already-existing functionality so I am reverting it on the 1.8 branch.

comment:17 Changed 6 years ago by Colin Snover

In [30161]:

Revert functionality change in patch release (verboten) and fix build profile to compensate. Refs #16421.

comment:18 Changed 6 years ago by bill

Milestone: 1.8.21.9
Owner: changed from bill to Rawld Gill
Status: reopenedassigned
Summary: cannot specify string in staticHasFeaturesspecifying string value in staticHasFeatures non-intuitive

I didn't notice the string part of the example in the beginning of util/build/transforms/hasFixup.js. Very odd, I wonder if there's a reason for forcing the user to both single and doublequote the string.

comment:19 Changed 6 years ago by Colin Snover

Presumably it is this way so you might reference an identifier instead, like a global settings object.

comment:20 Changed 6 years ago by bill

Oh, good point, OK I'll rollback my original checkin on trunk and do the nested quoting in webkitMobileProfile.

comment:21 Changed 6 years ago by Colin Snover

I wouldn’t say the revised approach on trunk is wrong; I would expect a string value to be by far the most common case, and if someone wanted to use an identifier they could provide some typed object instead that the build system recognises. I just don’t want to make a change like that in patch release since it significantly changes functionality.

comment:22 Changed 6 years ago by bill

Milestone: 1.92.0

Well, I think Boolean is the most common case, but likely String is more common than referencing a global variable.

Regardless, there's no good reason to break backwards compatibility here for 1.x. I'll mark this for possible inclusion into 2.0 and rollback my change for now. Rawld can either close as "wontfix" or check in the change for 2.0.

Last edited 6 years ago by bill (previous) (diff)

comment:23 Changed 6 years ago by bill

In [30176]:

Rollback [30108] as it turns out the old behavior (nested quoted of staticHasFeatures string values) was intended. Refs #16421.

comment:24 Changed 3 years ago by dylan

Milestone: 2.01.12
Resolution: patchwelcome
Status: assignedclosed

Given that no one has shown interest in creating a patch in the past 2+ years, I'm closing this as patchwelcome.

Note: See TracTickets for help on using tickets.