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

wip: include dynamic import automatically #714

Closed

Conversation

thiagoarrais
Copy link

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

This fixes #515 . The dynamic import syntax plugin has to be manually added to webpack config even though webpack itself already supports dynamic imports.

What is the new behavior?

Dynamic import syntax is automatically supported.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information:

Please let me know if and where I should add docs/tests.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Oct 29, 2018

I'd prefer that we enable it based on caller (e.g. caller.supportsDynamicImport), like we do for supportESModules).
Also, probably we should only enable it inside preset-env since it's the only package which enables anything automatically?

@thiagoarrais
Copy link
Author

@nicolo-ribaudo: I'm not familiar with this part of the code yet and may be misreading you here. Are you suggesting to move this change to @babel/preset-env?

@nicolo-ribaudo
Copy link
Member

Yes, but I'm not 100% sure about it so lets wait for someone else's opinion.

I'd prefer that we enable it based on caller (e.g. caller.supportsDynamicImport), like we do for supportESModules).

I think that we should do this in any case, though.

@hzoo hzoo requested a review from loganfsmyth October 30, 2018 21:18
@hzoo
Copy link
Member

hzoo commented Oct 30, 2018

Yes we introduced that flag to add this functionality, it's from #660

@thiagoarrais
Copy link
Author

Am I right to understand that https://github.com/babel/babel-loader/blob/7d8500c/src/injectCaller.js#L13 means that babel-loader already flags that dynamic imports are supported? And that we need to change preset-env to interpret this flag? If so, I'll close this PR and open another to preset-env.

@existentialism
Copy link
Member

@thiagoarrais yeah you're correct. I actually have a branch with the behavior enabled, but need to dig back into my notes on something I ran into (or some decision that needed to be made)

@loganfsmyth
Copy link
Member

I added supportsDynamicImport on the off chance we wanted it, but I think it's not obvious if we'll want to use it or what.

The main issue in my mind is that it seems surprising to automatically parse this syntax only when running in Webpack and not other contexts when it isn't standard yet. Once we get to the point of it being opt-in, I'm not sure it is clear why we'd go that route instead of expecting people to enable the syntax plugin manually.

@thiagoarrais
Copy link
Author

I will go on and try to implement this in preset-env as a learning exercise. We can later decide on merging it or not.

@existentialism: if you happen to find that branch, please let me know.

@loganfsmyth: I agree that it is surprising. But as I understand, that was exactly the intended behavior described in the original issue (#515). The project understanding can change, of course. Maybe the OP (@hzoo) can weigh in here?

Closing the PR for now to signal that I won't work on this change any longer. Please do keep the discussion going.

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.

[feature request] include dynamic import syntax plugin automatically
5 participants