Add support for new ES dynamic import()#143
Conversation
Codecov Report
|
|
Thanks for your contribution! Currently the plugin has some problems that have to be fixed, so probably before your feature can be merged those bugs have to be addressed first. It should be a matter of a few days. |
|
Sounds good, the 2.6-issue surely is more pressing after all |
|
@leoselig I think this should go on the beta branch. I'll give you heads up when it will be good to rebase this! Sorry for the confusion, but we want to avoid the situation from 2 weeks ago. |
|
@fatfisz No problem |
|
@leoselig Hi, the beta branch is in a good state now, so it's safe for you to rebase :) Please be sure to select Notice how a few things have changed with regards to how the method calls are handled. If you have any questions, I'm here to help! |
|
Had to close and re-open to fix appveyor issue (probably this one) |
- adds support for resolving module options on calls to the stage 3 proposal for `import()` (for details, see draft at https://github.com/tc39/proposal-dynamic-import) - this is very similar to the `Systom.import()`-implementation - we need to add `babel-preset-stage-2` in order to parse the `import()`-calls in our tests
|
@fatfisz (It did fit in quite well with the |
| "test": "jest --coverage", | ||
| "test:suite": "jest", | ||
| "test:watch": "jest --watch", | ||
| "test:watch": "jest --watch test/dynamicImport.test.js", |
| const isNormalCall = patterns.some(pattern => matchesPattern(state.types, calleePath, pattern)); | ||
|
|
||
| if (patterns.some(pattern => matchesPattern(state.types, calleePath, pattern))) { | ||
| if (isNormalCall) { |
There was a problem hiding this comment.
We should group these 2 conditions
| "babel-plugin-transform-es2015-modules-commonjs": "^6.24.0", | ||
| "babel-plugin-transform-object-rest-spread": "^6.23.0", | ||
| "babel-preset-env": "^1.2.2", | ||
| "babel-preset-stage-2": "^6.22.0", |
There was a problem hiding this comment.
Could you only use the required plugin instead?
- this replaces the `stage-2` preset that was pulling in way more than needed
|
I think this looks good :) I'd only make the paths & the test names consistent with the ones in |
tleunen
left a comment
tleunen
left a comment
There was a problem hiding this comment.
I'm all good once these small things are changed
| "pretest": "npm run lint", | ||
| "test": "jest --coverage", | ||
| "test:suite": "jest", | ||
| "test:watch": "jest --watch", |
There was a problem hiding this comment.
We still want to keep test:watch ;)
Sorry if this wasn't clear
| }); | ||
|
|
||
| it('should handle an empty path', () => { | ||
| const code = 'import(\'\').then(() => {}).catch(() => {});'; |
There was a problem hiding this comment.
I believe this is what @fatfisz mentioned. Use "" instead of \'\'
| ], | ||
| }; | ||
|
|
||
| it('CFG should resolve the path based on the root config', () => { |
There was a problem hiding this comment.
CFG not needed. Usually we start all tests with should.... Because it should... :)
|
Finally got around to fix the remarks |
|
Thanks a lot @leoselig. |
3 participants
stage 3 proposal for
import()(for details, see draft athttps://github.com/tc39/proposal-dynamic-import)
Systom.import()-implementationbabel-preset-stage-2in order to parse theimport()-calls in our tests