[T6933] Rename any shadowing Symbol binding

[amasad]

No description provided.

[hzoo]

👍

[codecov-io]

Current coverage is 85.21%

Merging #3250 into master will decrease coverage by -0.03% as of 635a066

@@            master   #3250   diff @@
======================================
  Files          215     215       
  Stmts        15699   15703     +4
  Branches      3360    3361     +1
  Methods          0       0       
======================================
- Hit          13383   13382     -1
- Partial        674     679     +5
  Missed        1642    1642       

Review entire Coverage Diff as of 635a066

Powered by Codecov. Updated on successful CI builds.

[loganfsmyth]

Should this just be a feature of Babel Core? This fixes this specific case, but this'll happen anywhere any global is shadowed. For instance

var Symbol = undefined;
var a;
a = [1, 2, 3];
for (var i of a){
    console.log(i);
}

would also throw for this reason.

I assume this same issue would happen with our use of Object and Promise, not sure if there are other globals we use in the helpers.

[amasad]

Good point, but are there times when it is expected that we use the
shadowed global? Like if someone wants to use their own promise library?

On Thursday, January 7, 2016, Logan Smyth notifications@github.com wrote:

In packages/babel-plugin-transform-es2015-typeof-symbol/src/index.js
#3250 (comment):

@@ -3,6 +3,14 @@ export default function ({ types: t }) {

return {
visitor: {

•  Scope({ scope }) {
  

•    if (!scope.getBinding("Symbol")) {
  

•      return;
  

•    }
  

•    scope.rename("Symbol");
  

Should this just be a feature of Babel Core? This fixes this specific
case, but this'll happen anywhere any global is shadowed. For instance

var Symbol = undefined;
var a;
a = [1, 2, 3];
for (var i of a){
console.log(i);
}

would also throw for this reason.

I assume this same issue would happen with our use of Object and Promise,
not sure if there are other globals we use in the helpers.

—
Reply to this email directly or view it on GitHub
https://github.com/babel/babel/pull/3250/files#r49159082.

[jdalton]

@amasad
If someone imported their Promise lib as Promise it would cause the same error Lodash was seeing with its Symbol module.

[loganfsmyth]

Valid point. If we want to only do Symbol for now, that's fine.

As @jdalton says, Promise is the one case that comes to mind. I don't think I'd say that's what I expect imported Promise to be used by helpers, but it's possible people are using this behavior and assuming it is correct. I guess if we were going to change Promise, we'd at least want to do it in a non-patch release. It definitely seems like a terrible idea if anyone is relying on that, since their code would break immediately if they used transform-runtime or external-helpers, which you could potentially want to introduce at any time.

[amasad]

Ok, let's ship this for now and open a task to track the larger issue.

[jdalton]

🚀