-
Notifications
You must be signed in to change notification settings - Fork 214
Consider case when 'core-js' is located in 'babel-polyfill' root #181
Conversation
Codecov Report
@@ Coverage Diff @@
## master #181 +/- ##
=======================================
Coverage 93.43% 93.43%
=======================================
Files 36 36
Lines 518 518
=======================================
Hits 484 484
Misses 34 34
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.
Just a couple changes and I think we can ship this.
@@ -99,6 +99,10 @@ module.exports = (neutrino) => { | |||
.chunkFilename('[id].[chunkhash].js') | |||
.end() | |||
.resolve | |||
.alias |
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.
Indent this line 2 more spaces.
@@ -99,6 +99,10 @@ module.exports = (neutrino) => { | |||
.chunkFilename('[id].[chunkhash].js') | |||
.end() | |||
.resolve | |||
.alias | |||
// TIP: make sure 2 version of "core-js" always match in package.json and babel-polyfill/package.json |
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.
- We can remove the "TIP" word here
- Capitalize "make" to "Make"
- Replace "version" with "versions"
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.
Yes. I thought what is the best here. "Make sure 2 versions of "core-js" always match in package.json and babel-polyfill/package.json" sounds like the next line of code do this. But it actually not. This comment should play a role of a reminder.
@@ -99,6 +99,10 @@ module.exports = (neutrino) => { | |||
.chunkFilename('[id].[chunkhash].js') | |||
.end() | |||
.resolve | |||
.alias | |||
// TIP: make sure 2 version of "core-js" always match in package.json and babel-polyfill/package.json | |||
.set('core-js', join(require.resolve('core-js'), '..')) |
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.
Instead of a join with ..
, let's use dirname
from path:
const { join, dirname } = require('path');
// ...
.set('core-js', dirname(require.resolve('core-js')))
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.
Agree. Should think of this from the very beginning.
This change fixes situation when there are more than one 'core-js' library versions in the project. In this case 'babel-polyfill' downloads its verison to '/node_modules/babel-polyfill/node_modules/core-js'. That means
babel-preset-env
can't correctly resolve paths to 'core-js' when processimport 'babel-polyfill'
. Babel will try to find it in '/node_modules/core-js' where the wrong version will be located.There is a particular project example when build fails because of core-js@1.x.x in the root node_modules and core-js@2.x.x in the babel-polyfill
![error](https://cloud.githubusercontent.com/assets/673144/25008379/2dc628be-206c-11e7-9b88-34752c4d174d.PNG)
![paths](https://cloud.githubusercontent.com/assets/673144/25008384/31f9695a-206c-11e7-9f45-2c69e40bf589.PNG)
This fix also expects that babel-polyfill structure is '/libs/index.js'. Hope it will not be changed.