Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

verify that property names after mangle are legal #1481

Merged
merged 1 commit into from Feb 12, 2017

Conversation

anatdagan
Copy link

same as in mangle_names (for variables/functions)

…that it is not a reserved word. This should apply to propsmangle as well.
@kzc
Copy link
Contributor

kzc commented Feb 10, 2017

I don't think this PR is necessary. Can you demonstrate a case where uglify-js@2.7.5 produces incorrect code?

@anatdagan
Copy link
Author

anatdagan commented Feb 12, 2017 via email

@avdg
Copy link
Contributor

avdg commented Feb 12, 2017

I still believe that there can be done more effort to spread the message (in real life, not by spammy and annoying deprecation notes) that older computers (windows 7 and especially older ones) should be replaced or in the rare case, upgraded.

@anatdagan
Copy link
Author

anatdagan commented Feb 12, 2017 via email

@mishoo
Copy link
Owner

mishoo commented Feb 12, 2017

I don't understand what issue does this fix. Can you describe?

@anatdagan
Copy link
Author

anatdagan commented Feb 12, 2017 via email

@mishoo mishoo merged commit eb55d8a into mishoo:master Feb 12, 2017
@mishoo
Copy link
Owner

mishoo commented Feb 12, 2017

Got it. Merged, thanks.

@anatdagan
Copy link
Author

anatdagan commented Feb 12, 2017 via email

@avdg
Copy link
Contributor

avdg commented Feb 12, 2017

(to avoid confusion, I just watched this from the phone and just attacked the statement involved old browsers, oh well)

@kzc
Copy link
Contributor

kzc commented Feb 12, 2017

@mishoo This PR is not required as such functionality can be supported by existing uglify options.

A property that happens to match a keyword is no different from any other property that a user may want to be excluded from --mangle-props.

Since properties matching keywords in IE8 have to be quoted anyway, reserving quoted properties in uglify-js@2.7.5 can be achieved by using --mangle-props=unquoted:

uglify-js@2.7.5:

$ echo 'obj.fi = obj["if"];' | uglifyjs -m -c --mangle-props=unquoted --support-ie8
obj.a=obj["if"];

Edit: My last post incorrectly used --screw-ie8 instead of --support-ie8.

@anatdagan
Copy link
Author

anatdagan commented Feb 12, 2017 via email

@anatdagan anatdagan mentioned this pull request Feb 12, 2017
@kzc
Copy link
Contributor

kzc commented Feb 12, 2017

It was about a regular [property] that was renamed to "if" by uglifyjs.

This PR is still not required.

Just use the uglify-js@2.7.5 --support-ie8 flag in the CLI or screw_ie8: false in minify().

A program to generate enough properties to make a property named "if" once mangled by uglify:

$ cat gen.js 

// generate enough properties to hit "if" once mangled...
console.log("o = {}");
for (var i = 0; i < 1000; ++i) {
    console.log("o.p" + i + " = " + i + ";");
}
$ node gen.js

o = {}
o.p0 = 0;
o.p1 = 1;
o.p2 = 2;
...
o.p997 = 997;
o.p998 = 998;
o.p999 = 999;
$ uglifyjs -V
uglify-js 2.7.5

with --screw-ie8:

$ node gen.js | uglifyjs -m -c sequences=0 --mangle-props -b --screw-ie8 | grep if

o.if = 331;

with --support-ie8:

$ node gen.js | uglifyjs -m -c sequences=0 --mangle-props -b --support-ie8 

o = {};
o.a = 0;
o.b = 1;
...
o.hf = 330;
o["if"] = 331;
o.jf = 332;
...
o.Br = 997;
o.Cr = 998;
o.Dr = 999;
$ node gen.js | uglifyjs -m -c sequences=0 --mangle-props -b --support-ie8 | grep if

o["if"] = 331;
$ node gen.js | uglifyjs -m -c sequences=0 --mangle-props -b --support-ie8 | grep '"'

o["if"] = 331;
o["in"] = 762;
o["do"] = 811;

Notice that the mangled property names matching keywords are quoted if you use the --support-ie8 flag for uglify-js@2.7.5

@anatdagan
Copy link
Author

anatdagan commented Feb 13, 2017 via email

@kzc
Copy link
Contributor

kzc commented Feb 13, 2017

@mishoo In light of the fact that --support-ie8 (a.k.a. screw_ie8: false) and/or --mangle-props=unquoted options will handle these cases in uglify-js@2.7.5, can this PR be reverted from master?

This PR negatively impacts minify size for non-IE platforms.

@kzc kzc mentioned this pull request Apr 3, 2017
alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Apr 3, 2017
Also handle `-Infinity` correctly.

reverts mishoo#1481
alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Apr 3, 2017
- fix handling of "-Infinity"
- add test case for "-0"

reverts mishoo#1481
alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Apr 3, 2017
- fix handling of "-Infinity"
- add test case for "-0"

reverts mishoo#1481
alexlamsl added a commit that referenced this pull request Apr 3, 2017
- fix handling of "-Infinity"
- add test case for "-0"

reverts #1481
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants