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] Safari: SyntaxError: Cannot declare a let variable twice r #1753

Closed
MichaelRando opened this issue Apr 1, 2017 · 9 comments

Comments

Projects
None yet
6 participants
@MichaelRando
Copy link

commented Apr 1, 2017

Re: #1612
My bundle.js has any number of let e = substitutions for my code let myVariableName =. This creates a shadow on the outer scope e. I don't know if it's a Safari bug where the scope of the two variables is let-correct or not (http://stackoverflow.com/questions/42450473/safari-syntaxerror-cannot-declare-a-let-variable-twice) but it would be easier to debug and safer in Safari if the variables were not so often 'let e'

input javascript:

class LotteryPicker extends React.Component {

  static compareNumbers(a, b) {
    return a - b;
  }

  constructor(props) {
    super(props);
    let pickedSets = [];
    for (let i = 0; i < 6; i++) {
      pickedSets.push({
        mainDrawNumbers: [],
        extraDrawNumbers: []
      }); // two groups per grid
    }
    this.state = {
      pickedSets: pickedSets,
      activeGrid: 0
    };
  }

  render() {
    return index.createElement('div', null);
  }
}

uglify syntax:

uglify({
      compress: {
        screw_ie8: true,
        warnings: false
      },
      output: {
        comments: false
      },
      sourceMap: true
    })

output javascript:

class ie extends oe.Component {
        static compareNumbers(e, t)
        {
            return e - t
        }
        constructor(e)
        {
            super(e);
            let t = [];
            for (let e = 0; e < 6; e++)
                t.push({
                    mainDrawNumbers: [],
                    extraDrawNumbers: []
                });
            this.state = {
                pickedSets: t,
                activeGrid: 0
            }
        }
        render()
        {
            return oe.createElement("div", null)
        }
    }

@alexlamsl alexlamsl added the harmony label Apr 1, 2017

@objectkit

This comment has been minimized.

Copy link

commented Apr 10, 2017

I was running into a similar issue and assumed it to be due to variable name collision produced by Uglify #harmony... I was pretty shocked that the code ran in Firefox and Chromium without this error. The development context was JavaScriptCore, which is assumably what Safari uses.

@khoanguyen2610

This comment has been minimized.

Copy link

commented Apr 11, 2017

I'm using angular 4.0 and javascript ES6 to develop project. I got error on safari. When i change ES6 to ES5, everything is ok, do not have any errors.
Waiting Uglify upgrade new version.

@kzc

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2017

Safari not supporting block scoping for let is a pretty basic ES2015 bug that should be reported to the WebKit project.

When someone creates a WebKit bug report for it, please post the link here.

@MichaelRando

This comment has been minimized.

Copy link
Author

commented Apr 11, 2017

I'm not convinced it's a webkit bug. I mean, even if it were, it's on every mac desktop and ios device, so dealing with it is the priority.

But I think the problem is indicated in the original stackoverflow comment, that the function argument is treated as a let variable. As a function argument, its block scope is the entire function. Reusing that variable as a loop scope shadow/alias is bad code and overlapped block scope. No?

@kzc

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2017

I'm not convinced it's a webkit bug.

Your stackoverflow link indicated it was a Safari 10 bug. One of the responders is a maintainer of Babel.

even if it were, it's on every mac desktop and ios device, so dealing with it is the priority.

If you want your javscript to reach the widest possible audience it should be in ES5 form using the release version of uglify.

The uglify harmony branch is experimental. Harmony compress and mangle is not reliable and these features should be disabled.

This project is a volunteer effort. Pull requests to workaround this Safari bug are welcome.

Perlmint pushed a commit to Perlmint/UglifyJS2 that referenced this issue Apr 28, 2017

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

Perlmint added a commit to Perlmint/UglifyJS2 that referenced this issue Apr 28, 2017

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

Perlmint added a commit to Perlmint/UglifyJS2 that referenced this issue Apr 28, 2017

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

Signed-off-by: Gyusun Yeom <omniavinco@gmail.com>

Perlmint added a commit to Perlmint/UglifyJS2 that referenced this issue Apr 28, 2017

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

Perlmint added a commit to Perlmint/UglifyJS2 that referenced this issue May 1, 2017

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

This comment has been minimized.

Copy link
Contributor

commented May 3, 2017

Perlmint added a commit to Perlmint/UglifyJS2 that referenced this issue May 10, 2017

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

Perlmint added a commit to Perlmint/UglifyJS2 that referenced this issue May 11, 2017

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

Perlmint added a commit to Perlmint/UglifyJS2 that referenced this issue May 11, 2017

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

Perlmint added a commit to Perlmint/UglifyJS2 that referenced this issue May 11, 2017

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

Perlmint added a commit to Perlmint/UglifyJS2 that referenced this issue May 11, 2017

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

Perlmint added a commit to Perlmint/UglifyJS2 that referenced this issue May 11, 2017

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

alexlamsl added a commit that referenced this issue May 11, 2017

fix safari syntax error - declare twice (#1851)
To avoid Safari bug, scope of for loop should enclose parent scope variables.


fixes #1753

@alexlamsl alexlamsl closed this May 11, 2017

@kzc

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

Reportedly fixed in Safari Technology Preview Release 31:

https://webkit.org/blog/7622/release-notes-for-safari-technology-preview-31/

Kagami added a commit to cutechan/cutechan that referenced this issue Jul 24, 2017

snuggs added a commit to snuggs/UglifyJS2 that referenced this issue 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

toddself added a commit to toddself/tinyify that referenced this issue Nov 6, 2017

Enable Safari 10 save mangling
Safari 10 has an issue with `let` and redeclarations; the only way to safely
let uglify mangle and generate a build capable of supporting Safari 10 is to
enable the specific Safari 10 option.

No.

Seriously.

mishoo/UglifyJS2#1753 (comment)

There is no noticable bundle size difference with this enabled

| type | safari10: true | safari10: false|
-----------------------------------------
| uncompressed | 494k | 489k |
| brotli  | 130k | 134k |
| gzip | 154k | 148k |
| deflate | 154k | 148k |

So somehow this is making Brotli more efficient. And the only browser that
doesn't support Brotli? Safari 10.

So win-win everywhere!

rroylance added a commit to rroylance/phaser-ce-npm-webpack-typescript-starter-project that referenced this issue Feb 13, 2018

v1.8.7
- Fix for Safari 10 bug with UglifyJS (mishoo/UglifyJS2#1753)
@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.

^ Judging from the number of linked commits above, there are probably many more people who are intermittently having this issue and haven't fixed it yet.

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