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

Prevent final resolution and facade creation for inlined dynamic imports #2677

Merged
merged 6 commits into from Feb 17, 2019

Conversation

Rich-Harris
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers: #2676

Description

This feels like a cheat — as if this PR fixes a symptom but not the underlying cause. I'm not familiar with this part of the codebase though; maybe it's okay? It fixes the real-world issue I was having.

@lukastaegert
Copy link
Member

Thanks for finding this issue and working out a possible solution. Based on this, I was able to dig to the root cause which is that rendering "final resolutions" should of course be restricted to cross-chunk dynamic imports. I added the changes + some test extensions to your PR.

While I was at it, I improved the solution so that the useless facade chunk is not created unless actually needed. In the future, I hope the logic can be changed so that facade chunks for dynamic imports will not be needed at all.

Please check if this works for you!

@lukastaegert lukastaegert changed the title Skip final resolution for already-rendered dynamic import Prevent final resolution and facade creation for inlined dynamic imports Feb 15, 2019
@Rich-Harris
Copy link
Contributor Author

Brilliant! The test suite that was failing is no longer doing so — thank you for the release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants