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

Don't check type annotations when deciding params scope #11349

Merged
merged 2 commits into from Apr 5, 2020

Conversation

nicolo-ribaudo
Copy link
Member

Q                       A
Fixed Issues? Fixes #11344
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Type annotations caused unnecessary usage of IIFE, because we checked if they referenced any value binding.

@nicolo-ribaudo nicolo-ribaudo added the PR: Polish 💅 A type of pull request used for our changelog categories label Mar 28, 2020
@nicolo-ribaudo nicolo-ribaudo added this to the v7.9.5 milestone Mar 28, 2020
@@ -36,6 +36,10 @@ const iifeVisitor = {
path.stop();
}
},
// type annotations don't use or introduce "real" bindings
"TypeAnnotation|TSTypeAnnotation"(path) {
Copy link
Contributor

@JLHwung JLHwung Mar 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we may expose bindingType of checkLVal to parseBindingList so we do not register bindings of lval when they are inside a type annotation.

BTW we should also skip TSTypeParameters.

function a(b = <T extends (c: any) => void>(): void => {}) {
  let c;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since identifiers are the most often created/moved nodes, I think that it would be safer to add a skip like this one to @babel/traverse rather than relying on what was initially parsed.

let c;
}

function d(e = <T>() => {}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This test will be passing without current change since we are not registering bindings when parsing type parameters.

@nicolo-ribaudo nicolo-ribaudo merged commit 65d09e4 into babel:master Apr 5, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the issue-11344 branch April 5, 2020 21:43
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Naming conflict in callback typing produces unnecessry complex state machine
4 participants