-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat: Migrate to npm@latest #58
Conversation
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.
👍
.travis.yml
Outdated
before_install: | ||
- nvm --version | ||
- node --version | ||
before_script: | ||
- |- | ||
if [ "$WEBPACK_VERSION" ]; then | ||
yarn add webpack@^$WEBPACK_VERSION | ||
npm i -g webpack@^$WEBPACK_VERSION |
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.
Why global install? The tests will still use the version installed locally.
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.
The behavior for npm i
has changed in node 8. If we were going to use a mix of versions, global was the only common denominator.
Now that we are using npm@5 for everything, npm i --no-save
is a viable option
src/tasks/travis.js
Outdated
@@ -19,7 +19,7 @@ module.exports = (config) => { | |||
include: [ | |||
{ | |||
os: 'linux', | |||
node_js: '7', | |||
node_js: '8', |
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 probably need a variable for this too ;-)
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.
Variables covering the entire supported nodejs range ..
"minNode": "4.3",
"latestNodeLTS": "6",
"latestNode": "8"
Just need to add the npm install in the before_install quick. |
src/tasks/travis.js
Outdated
if [[ npm -v != 5* ]]; then | ||
npm i -g npm@5; | ||
fi | ||
`.trim(), |
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.
.travis.yml
Outdated
@@ -19,8 +19,10 @@ matrix: | |||
node_js: '8' | |||
env: WEBPACK_VERSION="2.6.0" JOB_PART=coverage | |||
before_install: | |||
- npm i -g npm@latest |
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.
Very dangerous, the new version sometimes does not work as expected (example npm 5.0.0-5.0.1)
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've limited the install range to semver: Patch
. If it's a choice between occasionally breaking a patch version and having to pin vs. constantly having to update versions in 58 repositories, i'll take the former any day of the week personally.
12f5fb7
to
988dd71
Compare
@sapegin - Ship it |
Changes templates to execute via
npm
for all supported node version. Can be updated to install npm@latest for the 4.3 / 6 travis runs if we want.As this pertains to the contrib loaders & plugins, the proposal is to drop yarn and add an npm lock file which can then be used or not used depending on the developers installed nodejs / npm version with the exception of any dependency updates which will require the lock file to be updates.
I didn't add npm to the engines field ( think it's overkill ). Anyone updating packages will most likely be org-maintainer or admin, in which case requiring npm@5 to be installed is completely reasonable.
Closes webpack-contrib/organization#13