Skip to content

Safely update npm on Travis #42

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

Merged
merged 4 commits into from
Apr 11, 2019
Merged

Conversation

lukechilds
Copy link
Member

@lukechilds lukechilds commented Apr 9, 2019

This resolves most of the failing tests on Travis.

Resolves #33

On Travis we update npm before running any tests. The latest versions of npm use syntax that isn't available on the Node.js versions we test against so it kills the Travis instance before the tests even begin:

$ npm install -g npm
/home/travis/.nvm/versions/node/v5.12.0/bin/npm -> /home/travis/.nvm/versions/node/v5.12.0/lib/node_modules/npm/bin/npm-cli.js
/home/travis/.nvm/versions/node/v5.12.0/bin/npx -> /home/travis/.nvm/versions/node/v5.12.0/lib/node_modules/npm/bin/npx-cli.js
npm@6.9.0 /home/travis/.nvm/versions/node/v5.12.0/lib/node_modules/npm
/home/travis/.nvm/versions/node/v5.12.0/lib/node_modules/npm/bin/npm-cli.js:84
      let notifier = require('update-notifier')({pkg})
      ^^^
SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode

Just removing the npm updates form .travis.yml resolves this.

There are two remaining tests that still fail, however these are due to zuul and zuul-ngrok no longer running on these old Node.js versions.

zuul hasn't been updated in about a year and looks to have been replaced by airtap which is actively maintained.

@goto-bus-stop
Copy link
Member

goto-bus-stop commented Apr 9, 2019

the zuul-ngrok installation failure is because the version of npm that ships with 0.8 doesn't support ^4.0.0 caret syntax. you can replace the npm install -g npm@2 stuff by nvm install-latest-npm to get the most recent version of npm that will run on that node version (this is what we do in the browserify repo for example)

https://github.com/browserify/browserify/blob/7ad39ce835b6b3aa5718af04c8f57a5aeef6c636/.travis.yml

e; yes, we'll want to switch to airtap and run the browser tests from a recent node version instead :D

@lukechilds lukechilds changed the title Don't update npm on Travis Safely update npm on Travis Apr 9, 2019
@lukechilds
Copy link
Member Author

I've tried nvm install-latest-npm but it doesn't help. On Node.js 0.8 it installs npm@v4.5.0 and but then at npm install gives the error:

npm does not support Node.js v0.8.28

It's probably still a good idea to keep nvm install-latest-npm so we've always got the latest npm, would dropping Node.js 0.8 support/tests be an option?

Also, maybe you can clarify this:

It's not quite clear to me why this module is tested so extensively against old Node.js versions. Is it expected that anyone would actually use this in Node.js as opposed to the standard library assert?

Even if it is, do we need to support Node.js versions this old? Surely the main targets should be browser versions.

@BridgeAR
Copy link
Member

Is there a plan what Node.js versions should be supported? I believe the module requires a major bump if it's updated to the current assert version. So is there really a point in supporting versions < Node.js v6 (which is soon not supported anymore)?

@goto-bus-stop
Copy link
Member

right sorry, i meant to link to the entire before_install section:

before_install:
  # Old npm certs are untrusted https://github.com/npm/npm/issues/20191
- 'if [ "${TRAVIS_NODE_VERSION}" = "0.6" ] || [ "${TRAVIS_NODE_VERSION}" = "0.8" ]; then export NPM_CONFIG_STRICT_SSL=false; fi'
- 'nvm install-latest-npm'

generally for these modules we aim for maximum compatibility. originally, this module and things like util were probably(?) intended for use in node as well, but that's less important now. dropping those old versions would be a major change though which would be best to avoid for now until we're porting breaking changes from node.

@lukechilds
Copy link
Member Author

@goto-bus-stop That doesn't seem to help.

All that extra step is doing is setting NPM_CONFIG_STRICT_SSL=false for 0.8 which as already done by Travis here: https://travis-ci.org/browserify/commonjs-assert/jobs/518592923#L447

@goto-bus-stop
Copy link
Member

oh yikes. so it's just because zuul has a git dependency. previously when really old versions of node failed that change fixed it, but this time it's caused by something different 😓

Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, the remaining failures should be fixed by switching to airtap later.

@lukechilds
Copy link
Member Author

Cool, let me just remove the redundant NPM_CONFIG_STRICT_SSL=false before merging.

@lukechilds lukechilds merged commit 986fc8d into browserify:master Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Travis tests failing: bad npm installation
3 participants