-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
feat: Add upToScope
parameter to hasBinding
#17102
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
Conversation
liuxingbaoyu
commented
Jan 30, 2025
Q | A |
---|---|
Fixed Issues? | Fixes #17064 |
Patch: Bug Fix? | |
Major: Breaking Change? | |
Minor: New Feature? | |
Tests Added + Pass? | Yes |
Documentation PR Link | |
Any Dependency Changes? | |
License | MIT |
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/58665 |
) { | ||
if (!name) return false; | ||
const upToScope = (opts as { upToScope?: Scope })?.upToScope; |
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: Let's move opts
normalization here so we don't have to do the typecast.
if (scope.hasOwnBinding(name)) { | ||
return true; | ||
} | ||
} while ((scope = scope.parent)); | ||
|
||
if (!noUids && this.hasUid(name)) return true; | ||
if (!noGlobals && Scope.globals.includes(name)) return 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.
How do you think noGlobals
and noUids
should interact with the upToScope
option? The current behaviour is to completely ignore them when upToScope
is configured, as if they were both 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.
The updated behaviour looks good to me.