-
-
Notifications
You must be signed in to change notification settings - Fork 226
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
Add support for import() expressions #205
Conversation
Codecov Report
@@ Coverage Diff @@
## master #205 +/- ##
==========================================
+ Coverage 98.04% 98.04% +<.01%
==========================================
Files 28 29 +1
Lines 460 461 +1
==========================================
+ Hits 451 452 +1
Misses 9 9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @haggholm Thank you for your PR. Sorry that I review the code late. I left some comments, could you please help to take a look?
@@ -0,0 +1 @@ | |||
System.import('anyone'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the System.import
has been deprecated and we don't need to support it.
@@ -0,0 +1 @@ | |||
import('optimist'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please try the webpack named chunk feature (aka, the magic comment):
import(/* webpackChunkName: 'chunk' */ 'chunk')
I guess I was about as quick to reply as you were! I’m not 100% what you mean; do these changes reflect what you wanted? |
@haggholm Webpack's import(/* webpackChunkName: "my-chunk-name" */ 'module'); We should check if we are supporting it. |
Right. I added a test that verifies that it still detects the import -- I think that’s what you meant? I also cleaned up some cruft I’d accidentally introduced. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
Thank you for the contribution, @haggholm ! 🎉 |
* master: Upgrade all dependencies (#276) Fix error when Typescript is not installed (#282) Enable ES6/7 syntax in Typescript (#258) Update NPM tokens for deploy from Travis (#278) Recognize object arrays in Webpack module.rules.loaders (#233) Add support for import() expressions (#205) Bump Mocha to 5.x (#274) Fix ignore-bin-package default in ReadMe (#252) Fix typo: pasers -> parsers
Add support for lazy
import()
calls as used bySystem.import()
and lazy Webpack chunks.Modelled on the
require.resolve()
PR (#196). Not too familiar with the test framework, but copying tests, it seems to work (and mutating tests does properly break…).Perhaps this should be optional/disabled by default, but the options (esp. w.r.t. the tests) are a bit opaque to me: feedback extremely welcome.