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

Cannot redeclare let variable in Safari #559

Closed
tinovyatkin opened this issue May 26, 2017 · 10 comments · Fixed by #567
Closed

Cannot redeclare let variable in Safari #559

tinovyatkin opened this issue May 26, 2017 · 10 comments · Fixed by #567
Labels
has PR Has an open PR that fixes this issue

Comments

@tinovyatkin
Copy link

tinovyatkin commented May 26, 2017

After upgrading to babili 0.1.2 following error appears in Safari 10.1 (running ES6 native): SyntaxError: Cannot declare a let variable twice: 'e'.. It happens on fragments like this (after minify I have a lot of this):

function a(e) {
  let e = 1;
  console.log(e);
}
a(2);

// Real life minify result:

  function Ge(e, t) {
    const n = e.toLowerCase(),
          a = n.split(/\s+/),
          r = [];

    for (let e = 0, n = 0; n < t && e < a.length; n += a[e].length, e++) r.push(a[e].split('').map((e) => e.normalize('NFKD').charAt(0)).filter((e) => /[A-z]/.test(e)).join(''));
    return r.join('-');
  }

and it seems for me that Safari counts function parameters as declared by let. Neither Chrome, Firefox or Edge have such problem. Is there any workaround apart of switching off minify (as I'm forced to do now)?

@boopathi
Copy link
Member

boopathi commented May 26, 2017

Thanks. This looks like is a bug. It shouldn't be reused with fn param in the same scope.

Can you give the source code and config of how you used babili that reproduces this bug ?

I'm not able to reproduce with a simple function.

function foo(bar) {
  let baz = 1;
  console.log(baz);
}

gives

function foo(a) {
  let b = 1;
  console.log(b);
}

@tinovyatkin
Copy link
Author

The source code is huge (I pass babili over rollup bundle with mixed es6 / es3 content), and it produces a lot of that results, but all times with e letter, not sure if it make sense.

Config is below:

 // we will pass bundled file via babili
          ({ code, map } = transform(code, {
            babelrc: false,
            sourceMaps: true,
            presets: [
              [
  'env',
  {
    targets: {
      safari: '10.1',
      firefox: 53,
      chrome: 57,
      edge: 15,
    },
    modules: false,
    loose: true,
    useBuiltIns: true,
    preferBuiltins: true,
    debug: false,
  },
],
              [
                'babili',
                {
                  keepFnName: false,
                  keepClassName: false,
                  removeConsole: true,
                  removeDebugger: true,
                },
              ],
            ],
            compact: true,
            minified: true,
            comments: false,
            plugins: [
              [
                'transform-define',
                {
                  'tino.env': 'production',
                  'tino.es6': true,
                  'typeof window': 'object',
                },
              ],
            ],
          }));

the first function is just an example, the second function is output of this:

function createStub(str, maxLen) {
  const lc = str.toLowerCase();
  const stubsArray = lc.split(/\s+/);
  const res = [];
  for (
    let i = 0, currentLen = 0;
    currentLen < maxLen && i < stubsArray.length;
    (currentLen += stubsArray[i].length), i++
  ) {
    res.push(
      stubsArray[i]
        .split('')
        .map(a => a.normalize('NFKD').charAt(0))
        .filter(a => /[A-z]/.test(a))
        .join('')
    );
  }
  return res.join('-');
}

@boopathi
Copy link
Member

I think it is a safari bug.

function foo(a = 1) {
  for (let a = 2;;) {
    console.log("loop", a); // 2
    break;
  }
  console.log("fn param", a); // 1
}
foo();

is valid ES6. The let binds to the for loop and not the function. I'm really not sure how to work around this bug.

@kzc
Copy link

kzc commented May 26, 2017

@tinovyatkin
Copy link
Author

Nobody will write such code in real life, why minify should? Just take next letter if this is used for function param already?

@tinovyatkin
Copy link
Author

as per https://kangax.github.io/compat-table/es6/ Safari 10.1 is the browser with most complete ES6 support. If it has a bug, it's better to adopt for it, instead of fighting with a bleeding edge browser.

@Jessidhia
Copy link
Member

Safari's block scoping implementation is by far the buggiest. There is a lot of valid code that crashes in safari, both at parse and run time.

The only reason I haven't reported is because I haven't been able to get it to reproduce in non-proprietary code (or with bundles smaller than "entire work website codebase"), but hopefully this bug gets Safari to look better into their block scoping implementation.

@boopathi
Copy link
Member

FYI, https://webkit.org/blog/7622/release-notes-for-safari-technology-preview-31/ under JavaScript this bug is fixed in Safari.

@tinovyatkin
Copy link
Author

@boopathi That's great, but the bug is already in several shipped versions, so, I think it must be avoided for a next couple of years... No fix?

btmills added a commit to btmills/babili that referenced this issue Jun 6, 2017
Safari raises a syntax error for a `let` or `const` declaration in a
`for` loop initalization that shadows a parent function's parameter:

```js
function a(b) {
  let (b;;);
}
```

This is valid code, but Safari throws a syntax error. The bug has been
[reported](https://bugs.webkit.org/show_bug.cgi?id=171041) and since
[fixed](https://trac.webkit.org/changeset/217200/webkit/trunk/Source) in
WebKit, so future versions of Safari will not have this problem.

This modifies the scope tracker's `canUseInReferencedScopes` method to
detect cases where a rename would trigger this bug and use a different
name.

Fixes babel#559
btmills added a commit to btmills/babili that referenced this issue Jun 6, 2017
Safari raises a syntax error for a `let` or `const` declaration in a
`for` loop initalization that shadows a parent function's parameter:

```js
function a(b) {
  for (let b;;);
}
```

This is valid code, but Safari throws a syntax error. The bug has been
[reported](https://bugs.webkit.org/show_bug.cgi?id=171041) and since
[fixed](https://trac.webkit.org/changeset/217200/webkit/trunk/Source) in
WebKit, so future versions of Safari will not have this problem.

This modifies the scope tracker's `canUseInReferencedScopes` method to
detect cases where a rename would trigger this bug and use a different
name.

Fixes babel#559
btmills added a commit to btmills/babili that referenced this issue Jun 6, 2017
Safari raises a syntax error for a `let` or `const` declaration in a
`for` loop initalization that shadows a parent function's parameter:

```js
function a(b) {
  for (let b;;);
}
```

This is valid code, but Safari throws a syntax error. The bug has been
[reported](https://bugs.webkit.org/show_bug.cgi?id=171041) and since
[fixed](https://trac.webkit.org/changeset/217200/webkit/trunk/Source) in
WebKit, so future versions of Safari will not have this problem.

This modifies the scope tracker's `canUseInReferencedScopes` method to
detect cases where a rename would trigger this bug and use a different
name.

Fixes babel#559
@btmills
Copy link
Contributor

btmills commented Jun 6, 2017

We hit this bug at work. I've put together what I hope is a successful workaround and filed a PR: #567.

@boopathi boopathi added the has PR Has an open PR that fixes this issue label Jun 7, 2017
boopathi pushed a commit that referenced this issue Jun 13, 2017
* Add workaround for Safari for loop lexical scope bug

Safari raises a syntax error for a `let` or `const` declaration in a
`for` loop initalization that shadows a parent function's parameter:

```js
function a(b) {
  for (let b;;);
}
```

This is valid code, but Safari throws a syntax error. The bug has been
[reported](https://bugs.webkit.org/show_bug.cgi?id=171041) and since
[fixed](https://trac.webkit.org/changeset/217200/webkit/trunk/Source) in
WebKit, so future versions of Safari will not have this problem.

This modifies the scope tracker's `canUseInReferencedScopes` method to
detect cases where a rename would trigger this bug and use a different
name.

Fixes #559

* More idiomatic implementation

* More extensive tests

* Better use path API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has PR Has an open PR that fixes this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants