Skip to content

fix: register injected importDeclaration#10172

Merged
existentialism merged 2 commits intobabel:masterfrom
JLHwung:fix-export-default-from
Jul 6, 2019
Merged

fix: register injected importDeclaration#10172
existentialism merged 2 commits intobabel:masterfrom
JLHwung:fix-export-default-from

Conversation

@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jul 5, 2019

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

When transform-typescript combines with proposal-export-default-from, the export { _foo as foo }; will be erased as it seems that _foo is not defined in its own scope. This PR register the injected importDeclaration so that it can work well with elideImports from transform-typescript.

Note that #7592 uses scope.crawl() to manually sync nodes to AST. As crawl will incur great performance overkill, our approach check the injected import specifier only and register when necessary.

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 5, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/11064/

@JLHwung JLHwung force-pushed the fix-export-default-from branch from eac1b1c to c301efb Compare July 5, 2019 21:36
@JLHwung JLHwung force-pushed the fix-export-default-from branch from c301efb to c092c1e Compare July 5, 2019 21:52
@JLHwung JLHwung changed the title fix: export should not be erased when mixed with `proposal-export-def… fix: register injected importDeclaration Jul 5, 2019
@nicolo-ribaudo nicolo-ribaudo added this to the v7.5.2 milestone Jul 6, 2019
@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories i: regression labels Jul 6, 2019
},
ImportDefaultSpecifier(path) {
if (!path.scope.hasOwnBinding(path.node.local.name)) {
path.scope.registerDeclaration(path.parentPath);
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to do this as soon as the import declaration is created, in the ExportNamedDeclaration visitor. This makes it clearer why we are doing it, and it prevents other plugin from running between the creating the declaration and registering it.

path.replaceWithMultiple(nodes); returns an array containing the new inserted paths: the first one should be the import declaration, and you can get it's default specifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

path.replaceWithMultiple(nodes) returns an array containing the new inserted paths.

Yes it is. It seems that @types/babel__traverse declares out-of-dated signature here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

i: regression outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants