babel/minify

Cannot redeclare let variable in Safari

tinovyatkin opened this issue · 10 comments

tinovyatkin commented

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 commented

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 commented

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 commented

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.

tinovyatkin commented

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

tinovyatkin commented

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 commented

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.

tinovyatkin commented

@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 commented

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

  • 👍3
  • 🎉1