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

[polymer-build] #3402 resolveBareSpecifiers must be a Program #3404

Merged
merged 2 commits into from
Apr 25, 2019

Conversation

btopro
Copy link
Contributor

@btopro btopro commented Apr 19, 2019

Babel's life cycle appears to make the following combination not possible with the current configuration of polymer's build process.

  • "moduleResolution": "node"
  • "transformModulesToAmd": true
  • "bundle": false,

What ends up happening now:

  • Babel processes the tree and transforms ast for AMD compilation and/or ES5 syntax in the Program level callback
  • This happens PRIOR to processing babel-plugin-bare-specifiers resolveBareSpecifiers export because it is running in a CallExpression which fires later
  • The CallExpression seeks out things marked 'Import' but because the compiling has already happened, it can't find everything
  • Dynamic imports get borked as a result, not being correctly resolved to their actual location in node_modules because it doesn't process it as an Object tree but instead as compiled code.

This change fixes this by..

This shifts it to process at the Program level and traverse the path to execute CallExpression's. This should ensure that the timing is correct so that the Objects can be resolved to their correct node_modules / relative location, prior to the ES5 / AMD transformations processing the tree into text.

Gains via this PR

  • Users will be able to user polymer.json as expected
  • Polymer build will correctly resolve dynamic imports regardless of compiling or module resolution used

Outstanding issues

#3403 is most likely related to this but I haven't been able to see what the issue is as to why this wouldn't overlap.

@LarsDenBakker
Copy link
Contributor

Running into this issue as well. Thanks for making the PR!

@Westbrook
Copy link

@usergenic Using this update locally solved this issue for me as well. If you like what you see, too, I hope we can get a merge and release scheduled for early next week.

@usergenic
Copy link
Contributor

Hey guys - I'm just catching up after holiday weekend. Saw your slack thread and then came over here. Am reviewing now.

I'm actually not as familiar with the build babel transforms and my first reaction is, why do we do compilation before resolving these specifiers; seems like we should be resolving specifiers first...

Anyways looking at this now. Thanks for this work, @btopro!

@usergenic
Copy link
Contributor

I'm still working on writing a test to repro the problem and include it as a regression test, but if you want to check to see that this solves issue for you, I've got a simple PR that reordered the specifier transform plugin #3405

@usergenic
Copy link
Contributor

Our dynamic import AMD transform is causing the problem because it is written as a Program visitor that does its own traversal, essentially making it always perform its own CallExpression transforms first. After a couple hours of attempting to refactor etc, it turns out your approach here is the most reasonable solution, so I'm going to go with this one. Great job :)

@usergenic usergenic merged commit 9e990ae into Polymer:master Apr 25, 2019
usergenic added a commit that referenced this pull request Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants