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
Define default value for vars shadowing params #11307
Define default value for vars shadowing params #11307
Conversation
@@ -1,4 +1,3 @@ | |||
import callDelegate from "@babel/helper-call-delegate"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now unused and can be removed from the repo.
This prevents this plugin from running after the destructuring transform, which causes babel#11256
I ended up also pushing a workaround for #11256 (a fix would require major changes to how we handle the scope) |
@@ -1,5 +1,5 @@ | |||
function foo(a = 2) { | |||
for (var a, i = 0; i < 1; i++); | |||
expect(a).toBe(undefined); | |||
expect(a).toBe(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
|
||
for (let i = 0; i < params.length; i++) { | ||
const param = params[i]; | ||
let isSimpleParameterList = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
const isSimpleParameterList = params.every(param => param.isIdentifier());
It will return true
when params
is empty.
scope: scope, | ||
stop: false, | ||
needsOuterBinding: false, | ||
shadowedParams: new Set(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move shadowedParams
out of state
if we don't intend it accessed by visitIdentifier
.
@@ -0,0 +1,6 @@ | |||
function test(date, defValue = 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: lol they don't have to be date
and defValue
.
|
||
const iifeVisitor = { | ||
ReferencedIdentifier(path, state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can simplify to
const iifeVisitor = {
"ReferencedIdentifier|BindingIdentifier": visitIdentifier,
}
If there is a BindingIdentifier in the function scope, it should not satisfy scope.getBinding(name) === state.scope.parent.getBinding(name)
so essentially it will be noop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 quiz ftw :)
!isSafeBinding(scope, node) || | ||
!isSafeBinding(state.scope, node) | ||
name === "eval" || | ||
(scope.getBinding(name) === state.scope.parent.getBinding(name) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this just a !scope.hasOwnBinding
check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, state.scope
does not equal to scope
.
… ci] Co-Authored-By: Brian Ng <bng412@gmail.com>
The best way to handle params's horrible shadowing behavior is by... relying on it 😁