Skip to content

Visit default export expressions in module visitor #18977

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

Merged

Conversation

weswigham
Copy link
Member

Fixes #18968

@weswigham weswigham requested review from rbuckton and mhegazy October 5, 2017 22:11
@mhegazy
Copy link
Contributor

mhegazy commented Oct 5, 2017

Can you do an audit on this file, and make sure we are transpiling imports in all places, this is the third issue we have had to deal with regarding this.

@weswigham
Copy link
Member Author

weswigham commented Oct 5, 2017

@mhegazy I just grep'd the file for all node. instances, I found one more obvious issue involving nested imports (await import(await import("foo")), opened #18982 - issue seems to actually be with the generator transform, but it may compound with the module transform), and then by chance also found #18981. Besides those, at this point it looks like all remaining node references in the module transformer are either leaf nodes (modifiers, operators), or get visited. The whole "manually interleave multiple logical visitors to attempt to get better perf" makes it difficult to track. It'd be nicer if we just had a function for composing independent transforms into a single walk.

@weswigham weswigham merged commit 7a4c331 into microsoft:master Oct 5, 2017
@weswigham weswigham deleted the dynamic-import-on-default-exports branch October 5, 2017 23:47
@weswigham
Copy link
Member Author

Also, @mhegazy does this need to be ported to the 2.6 branch?

@DanielRosenwasser
Copy link
Member

@weswigham yup, it'll just go after the RC.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 6, 2017

Also, @mhegazy does this need to be ported to the 2.6 branch?

I will do a merge from master next week.

@weswigham
Copy link
Member Author

Got it, won't port anything explicitly until then, then.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants