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

[ES6] fix safari syntax error - declare twice #1851

Merged
merged 3 commits into from May 11, 2017

Conversation

@Perlmint
Copy link

commented Apr 28, 2017

fix issue #1753
by enclosing variables of parent scope, avoid safari error.

@@ -122,6 +122,14 @@ AST_Toplevel.DEFMETHOD("figure_out_scope", function(options){
scope.uses_eval = save_scope.uses_eval;
scope.directives = save_scope.directives;
}
// issue 1753 - name mangling in for loop scope should consider parent scope

This comment has been minimized.

Copy link
@kzc

kzc Apr 28, 2017

Contributor

Please mention:

// Safari/Webkit bug workaround - loop init let variable shadowing argument.

For bonus points if you have the webkit bug URL, please add it here.

This comment has been minimized.

Copy link
@kzc

kzc Apr 28, 2017

Contributor

Instead of "issue 1753" please include complete URL of issue:

// https://github.com/mishoo/UglifyJS2/issues/1753
@kzc

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2017

With this PR:

$ echo 'function f(h){for(let i of a) for(let j of b) console.log(i,j); console.log(h)}' | bin/uglifyjs -b bracketize -m
function f(o) {
    for (let n of a) {
        for (let o of b) {
            console.log(n, o);
        }
    }
    console.log(o);
}
$ echo 'class C{f(h,a,b){for(let i of a)for(let j of b)console.log(i,j);console.log(h)}}' | bin/uglifyjs -b bracketize -m
class C {
     f(o, n, l) {
        for (let c of n) {
            for (let o of l) {
                console.log(c, o);
            }
        }
        console.log(o);
    }
}

let o shadows the argument o in cases above - will they run without error in buggy Safari 10?

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Apr 28, 2017

If I understand correctly, this PR:

  • alters the behaviour of mangle indiscriminately
  • aims to fix an issue that only applies to Safari

I'd prefer this special workaround to be gated by a new flag similar ie8, plus limiting the code changes to mangle_names() if possible. I don't think the current changes to AST_Scope.variables are correct and/or safe, as you will end up with the same SymbolDef instance being interpreted as belonging to multiple AST_Scope (and since compress also relies on figure_out_scope(), this won't end well...)

Please refer to the existing ie8 code which works around catch(e) scope-related issues.

@kzc

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2017

@alexlamsl I have the same concerns.

Also, I don't see any code specific to function arguments and let/const variables which is apparently the source of the problem.

I'd also like to see a reference to the Safari/WebKit bug to understand the true nature of the issue. The Safari/WebKit bug does not appear to be well understood.

@Perlmint Perlmint force-pushed the Perlmint:harmony branch from bdf21eb to 116eb8a May 1, 2017

@Perlmint Perlmint changed the title [ES6] fix safari syntax error - declare twice [WIP] [ES6] fix safari syntax error - declare twice May 1, 2017

@Perlmint Perlmint force-pushed the Perlmint:harmony branch 3 times, most recently from 7e16b9c to 0d3f94c May 1, 2017

@Perlmint

This comment has been minimized.

Copy link
Author

commented May 3, 2017

First of all, thanks to fast and specific reviews.

@kzc I edited comments like you wrote. Codes you wrote doesn't occur error on Safari10(OSX).

@alexlamsl I tried to deal with this issue by editing only mangle_names, but I can't find way to know whether AST_Scope is created from For like nodes - For, ForIn, ForOf.
Also, if I understand correctly, already SymbolRef is shared by multiple scopes, mark_enclosed function can push same SymbolRef instance into scopes. so, I think it could not be the problem.

I just added flag support_safari10, logic is not changed from previous my commits.

@Perlmint Perlmint changed the title [WIP] [ES6] fix safari syntax error - declare twice [ES6] fix safari syntax error - declare twice May 3, 2017

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2017

Putting aside where the logic should reside, the latest changes still doesn't look right because AST_Scope.variables would not have been fully populated when you push to scope.enclosed. You are exactly within the TreeWalker where we in the process of populating that array.

Also note that harmony is currently still on 2.x branch, and will soon merge with master after 3.0.0 is released. In the latest version, screw_ie8 is renamed simply to ie8, so you can use safari10 as the flag name instead. bin/uglifyjs also underwent a major overhaul.

@kzc

This comment has been minimized.

Copy link
Contributor

commented May 3, 2017

@Perlmint Thanks for including the Webkit bug link:

https://bugs.webkit.org/show_bug.cgi?id=171041

@fractalzombie

This comment has been minimized.

Copy link

commented May 4, 2017

Want this PR

@Perlmint Perlmint force-pushed the Perlmint:harmony branch from 0d3f94c to cd1d9c3 May 10, 2017

@Perlmint

This comment has been minimized.

Copy link
Author

commented May 10, 2017

@alexlamsl Thanks to precise comment. I just committed fixed one.

I'm sorry to late response. but My macbook was broken, I couldn't work fewdays.

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2017

@Perlmint no worries and take your time 😉

I think you need to rebase this PR before pushing those changes, as it is currently still based on 2.x and I can't merge at all due to conflicts.

pure_getters : !false_by_default && "strict",
pure_funcs : null,
reduce_vars : !false_by_default,
support_safari10 : true,

This comment has been minimized.

Copy link
@kzc

kzc May 10, 2017

Contributor

Please change support_safari10 to safari10.

This comment has been minimized.

Copy link
@kzc

kzc May 10, 2017

Contributor

...and revert all the row formatting of options unrelated to this PR.

// pass 4: add symbol definitions to loop scopes
// Safari/Webkit bug workaround - loop init let variable shadowing argument.
// https://github.com/mishoo/UglifyJS2/issues/1753
// related webkit bug ticket: https://bugs.webkit.org/show_bug.cgi?id=171041

This comment has been minimized.

Copy link
@kzc

kzc May 10, 2017

Contributor

Please change line above to simply:

    // https://bugs.webkit.org/show_bug.cgi?id=171041

@Perlmint Perlmint force-pushed the Perlmint:harmony branch from cd1d9c3 to 255e14b May 11, 2017

@Perlmint

This comment has been minimized.

Copy link
Author

commented May 11, 2017

@alexlamsl rebased.
@kzc Thanks a lot. I missed those points.

@@ -35,6 +35,7 @@ program.option("--comments [filter]", "Preserve copyright comments in the output
program.option("--config-file <file>", "Read minify() options from JSON file.");
program.option("-d, --define <expr>[=value]", "Global definitions.", parse_js("define"));
program.option("--ie8", "Support non-standard Internet Explorer 8.");
program.option("--no-safari10", "Do not support Safari 10 by not applying patch about For loop related scope issue.")

This comment has been minimized.

Copy link
@alexlamsl

alexlamsl May 11, 2017

Collaborator

--safari10?

@@ -17,7 +17,7 @@ function read_source_map(code) {
}

function set_shorthand(name, options, keys) {
if (options[name]) {
if (options[name] != null) {

This comment has been minimized.

Copy link
@alexlamsl

alexlamsl May 11, 2017

Collaborator

What is the reason for changing this?

@@ -36,6 +36,7 @@ function minify(files, options) {
options = defaults(options, {
compress: {},
ie8: false,
safari10: true,

This comment has been minimized.

Copy link
@alexlamsl

alexlamsl May 11, 2017

Collaborator

Default should be false, just like ie8. These are special case options, so don't force it upon everyone.

This comment has been minimized.

Copy link
@Perlmint

Perlmint May 11, 2017

Author

I just thought that safari10 option should be enabled default because it is latest version. so I made changes -you commented previous- in same reason.
if default should be false, other changes are also not needed.
I'll change it to false

@@ -46,6 +47,7 @@ function minify(files, options) {
wrap: false,
}, true);
set_shorthand("ie8", options, [ "compress", "mangle", "output" ]);
set_shorthand("safari10", options, [ "mangle" ]);

This comment has been minimized.

Copy link
@alexlamsl

alexlamsl May 11, 2017

Collaborator

In fact, why should it even be a toplevel option, if only mangle is affected?

Just keep it under options.mangle.safari10 for now. If it turns out Safari becomes the next IE6 in terms of quirks, we can add a toplevel option then. Right now it just add noise to the global namespace.

// Safari/Webkit bug workaround - loop init let variable shadowing argument.
// https://github.com/mishoo/UglifyJS2/issues/1753
// related webkit bug ticket: https://bugs.webkit.org/show_bug.cgi?id=171041
if (node instanceof AST_For ||

This comment has been minimized.

Copy link
@kzc

kzc May 11, 2017

Contributor

Please wrap this if with:

    if (options.safari10) {
...
    }
@@ -122,6 +124,14 @@ AST_Toplevel.DEFMETHOD("figure_out_scope", function(options){
scope.uses_eval = save_scope.uses_eval;
scope.directives = save_scope.directives;
}
// Safari/Webkit bug workaround - loop init let variable shadowing argument.
// https://github.com/mishoo/UglifyJS2/issues/1753
// related webkit bug ticket: https://bugs.webkit.org/show_bug.cgi?id=171041

This comment has been minimized.

Copy link
@kzc

kzc May 11, 2017

Contributor

We don't need this 3 line comment here as it's repeated below.

@Perlmint Perlmint force-pushed the Perlmint:harmony branch 2 times, most recently from cbd14d6 to 9621b0c May 11, 2017

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2017

@Perlmint so given the scope of this workaround, I think we don't need any new toplevel options at all.

Which means no changes required for bin/uglifyjs or lib/minify.js - it is just a new option within mangle, so you can already access this via uglify-js -m safari10 and minify(code, {mangle{safari10: true}})

Edit: sorry I missed your latest commits - it looks fine now 👍

@kzc

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

Even without understanding the fine points, all the code is behind options.safari10 with the flag defaulted to false, so there's no risk.

LGTM

@alexlamsl
Copy link
Collaborator

left a comment

Two more minor nits, and we are good to go!

@@ -54,6 +54,7 @@ function minify(files, options) {
cache: null,
eval: false,
ie8: false,
safari10: false,

This comment has been minimized.

Copy link
@alexlamsl

alexlamsl May 11, 2017

Collaborator

Can you move this down above toplevel so as to keep this section in alphabetical order?

@@ -102,6 +102,7 @@ AST_Toplevel.DEFMETHOD("figure_out_scope", function(options){
options = defaults(options, {
cache: null,
ie8: false,
safari10: false

This comment has been minimized.

Copy link
@alexlamsl

alexlamsl May 11, 2017

Collaborator

Please add a comma here so as to minimise diff in future expansions.

@Perlmint Perlmint force-pushed the Perlmint:harmony branch from 9621b0c to 5414aa1 May 11, 2017

@Perlmint

This comment has been minimized.

Copy link
Author

commented May 11, 2017

@alexlamsl fixed! Thanks.

@@ -58,6 +58,7 @@ function minify(files, options) {
properties: false,
reserved: [],
toplevel: false,
safari10: false,

This comment has been minimized.

Copy link
@alexlamsl

alexlamsl May 11, 2017

Collaborator

Above, not below... 😅

Between reserved and toplevel.

This comment has been minimized.

Copy link
@Perlmint

Perlmint May 11, 2017

Author

Oh... sorry. I'll fix it now.

if (options.safari10) {
if (node instanceof AST_For ||
node instanceof AST_ForIn ||
node instanceof AST_ForOf) {

This comment has been minimized.

Copy link
@kzc

kzc May 11, 2017

Contributor

Since:

var AST_ForOf = DEFNODE("ForOf", null, {
    $documentation: "A `for ... of` statement",
}, AST_ForIn);

you can remove the AST_ForOf condition.

This comment has been minimized.

Copy link
@kzc

kzc May 11, 2017

Contributor

and the if can be a single line.

This comment has been minimized.

Copy link
@Perlmint

Perlmint May 11, 2017

Author

You mean, suggest merging two if statement?

This comment has been minimized.

Copy link
@kzc

kzc May 11, 2017

Contributor

I prefer keeping the if (options.safari10) separate to better show that code is definitely not run if the option is not in effect without any thinking.

This comment has been minimized.

Copy link
@kzc

kzc May 11, 2017

Contributor

I'm a big fan of not thinking.

This comment has been minimized.

Copy link
@alexlamsl

alexlamsl May 11, 2017

Collaborator

@Perlmint so I think @kzc meant:

if (options.safari10) {
    if (node instanceof AST_For || node instanceof AST_ForIn) {
...
@kzc

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

@Perlmint Thank you for contributing this patch and your patience in this review exercise.

@Perlmint Perlmint force-pushed the Perlmint:harmony branch from 5414aa1 to aa17da4 May 11, 2017

Perlmint added some commits Apr 28, 2017

fix safari syntax error - decalring twice
fix #1753.
to mangle names properly - to avoid safari error, scope of for loop should enclose parent scope variables

@Perlmint Perlmint force-pushed the Perlmint:harmony branch from aa17da4 to 0976fc3 May 11, 2017

@Perlmint

This comment has been minimized.

Copy link
Author

commented May 11, 2017

@kzc looks better. Thanks.

@alexlamsl alexlamsl merged commit fcd90db into mishoo:harmony May 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@snuggs

This comment has been minimized.

Copy link

commented Aug 22, 2017

@alexlamsl @kzc @Perlmint quick question guys..

  1. How do I enable from CLI?
  2. Do you know why minify seems to be running twice per iteration? (I have 2 uglifyjs calls 4 outputs when console.logging from lib/minify.js
  3. Should I still be getting same let e issue on Safari after using safari10? Please see image
    capture d ecran 2017-08-22 a 12 55 43
    below.
@kzc

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2017

$ cat t1753.js

        class SomeClass {
            constructor(props) {
                let pickedSets = [];
                for (let i = 0; i < 6; i++) {
                    pickedSets.push({
                        mainDrawNumbers: [],
                        extraDrawNumbers: []
                    });
                }
            }
        }
$ bin/uglifyjs -V
uglify-es 3.0.28

The Safari 10 loop bug workaround is disabled by default:

$ bin/uglifyjs t1753.js -m safari10=false -b
class SomeClass {
    constructor(r) {
        let s = [];
        for (let r = 0; r < 6; r++) {
            s.push({
                mainDrawNumbers: [],
                extraDrawNumbers: []
            });
        }
    }
}

Safari 10 loop bug workaround enabled:

$ bin/uglifyjs t1753.js -m safari10=true -b
class SomeClass {
    constructor(r) {
        let s = [];
        for (let a = 0; a < 6; a++) {
            s.push({
                mainDrawNumbers: [],
                extraDrawNumbers: []
            });
        }
    }
}

Do you know why minify seems to be running twice per iteration?

minify also parses command line options in bin/uglifyjs among other things.

snuggs added a commit to snuggs/UglifyJS2 that referenced this pull request Aug 22, 2017

Update Readme.md `mangle` settings
These are based off https://github.com/Perlmint/UglifyJS2/blob/0976fc3e4b303be6de8a366f8406d8906b5521e1/lib/minify.js#L54-L61
And had no documentation update from this issue mishoo#1753 and this PR mishoo#1851.

Discovered when working on this Pull Request devpunks/snuggsi#78

Please let me know where i can better update copy text. Difficult to know what the flags do without a readme for the flags. ;-)

Thanks!

/cc @brandondees @robcole @angelocordon

blowery added a commit to Automattic/wp-calypso that referenced this pull request Jul 14, 2018

blowery added a commit to Automattic/wp-calypso that referenced this pull request Jul 15, 2018

Build: Add work-around for safari 10 let bug (#26068)
* Build: Add work-around for safari 10 let bug

See mishoo/UglifyJS2#1851

* Add the mangle option to the right minifier. Remove the redundant one.

* change the opt extension to min to cache break the uglify change
@nylen

This comment has been minimized.

Copy link

commented Jul 15, 2018

Hi everyone, can I ask why this option was not enabled by default? ie8 is one thing, but safari10 is still a "modern" browser, and this is a pretty terrible bug to run into and debug, since for most of us it is only going to appear in production and only with certain builds.

mmtr added a commit to Automattic/wp-calypso that referenced this pull request Jul 16, 2018

Build: Add work-around for safari 10 let bug (#26068)
* Build: Add work-around for safari 10 let bug

See mishoo/UglifyJS2#1851

* Add the mangle option to the right minifier. Remove the redundant one.

* change the opt extension to min to cache break the uglify change

by-robots pushed a commit to Automattic/wp-calypso that referenced this pull request Jul 19, 2018

Build: Add work-around for safari 10 let bug (#26068)
* Build: Add work-around for safari 10 let bug

See mishoo/UglifyJS2#1851

* Add the mangle option to the right minifier. Remove the redundant one.

* change the opt extension to min to cache break the uglify change
@nvcken

This comment has been minimized.

Copy link

commented Sep 12, 2018

is this fix apply on master branch ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.