This repository was archived by the owner on Aug 7, 2021. It is now read-only.
fix: some packages are treated as externals when they shouldn't #771
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In case `externals` are passed to webpack.config.js, they are converted to regular expression. The idea is to detect all imports for this module, for example if it is `nativescript-vue`, we would like to make externals imports like `nativescript-vue/somesubdir/file1`. However, the regular expression we construct also matches packages like `nativescript-vue-template`. Fix the regular expressions in all template webpack.config.js files and add unit tests for them. Add devDependency to `proxyquire` as the webpack configs that are tested, require some packages that are not available in node_modules of `nativescript-dev-webpack` (including the `nativescript-dev-webpack` itself). This is expected for the `webpack.config.js` files, as they should be placed in the NativeScript applications, so mocking the requires shouldn't be a problem in this case.
|
run ci |
1 similar comment
|
run ci |
|
test |
sis0k0
reviewed
Jan 26, 2019
There was a problem hiding this comment.
I thought it might be better to construct the regular expressions from the externals strings directly in compiler.js and not in the webpack configs. What do you think?
templates/webpack.angular.js
Outdated
| @@ -48,7 +48,7 @@ module.exports = env => { | |||
| } = env; | |||
| env.externals = env.externals || []; | |||
| const externals = (env.externals).map((e) => { // --env.externals | |||
| return new RegExp(e + ".*"); | |||
| return new RegExp(`^${e}((/.*)|$)`); | |||
There was a problem hiding this comment.
Could you add a comment what the regex does, and why we need the externals array?
Move the conversion of regular expressions from `string[]` to `RegExp[]` to index.js file, so it can be reused in all webpack.config.js files. Move most of the tests to the `index.spec.js` due to this change.
|
Hey @sis0k0 , I'm unable to move the Regular Expression generation to compiler.js as it should be read by the webpack process which is spawned there. However, I've moved the logic to |
sis0k0
suggested changes
Jan 30, 2019
|
@sis0k0 thanks a lot for the suggestions, I've commited all of them |
sis0k0
approved these changes
Feb 1, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In case
externalsare passed to webpack.config.js, they are converted to regular expression. The idea is to detect all imports for this module, for example if it isnativescript-vue, we would like to make externals imports likenativescript-vue/somesubdir/file1.However, the regular expression we construct also matches packages like
nativescript-vue-template.Fix the regular expressions in all template webpack.config.js files and add unit tests for them.
Add devDependency to
proxyquireas the webpack configs that are tested, require some packages that are not available in node_modules ofnativescript-dev-webpack(including thenativescript-dev-webpackitself). This is expected for thewebpack.config.jsfiles, as they should be placed in the NativeScript applications, so mocking the requires shouldn't be a problem in this case.PR Checklist
What is the current behavior?
Some packages may be treated as externals when they shouldn't (they are not passed in the env.externals array).
What is the new behavior?
Only packages passed as externals are treated as externals.
Fixes/Implements/Closes #[Issue Number].