browserify / commonjs-assert

Node.js's require('assert') for all engines
MIT License
293 stars 57 forks source link

Safely update npm on Travis #42

Closed lukechilds closed 5 years ago

lukechilds commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 :sweat:

lukechilds commented 5 years ago

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