Skip to content
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

ci: test Node.js 6, 8, 10 and 11 #908

Closed
wants to merge 1 commit into from
Closed

ci: test Node.js 6, 8, 10 and 11 #908

wants to merge 1 commit into from

Conversation

DanielRuf
Copy link
Contributor

Test only Node.js 6, 8, 10 (LTS) and stable (11).

https://github.com/nodejs/Release/blob/master/README.md

@nfischer
Copy link
Member

@DanielRuf
Copy link
Contributor Author

We probably won't drop v7/v9

But why? They reached their EOL.

@DanielRuf
Copy link
Contributor Author

We can't use npm install, we have our own script (see #896)

This produces different results in CI and production (lockfiles are not working there).

We chose c++ as a language a long time ago, and I can't remember why (so I can't tell you if the reason is still relevant). I think it was just to get finer-grained control over our CI steps

But it does not seem to be used.
I'll check and update the CI script.

@nfischer
Copy link
Member

But why? They reached their EOL.

I'm not aware of a compelling argument to support v6 but not v7. If we support v7, we should probably test it too.


Did some digging, and it seems like we switched to language: c++ in order to support OS X (#283). I assume Travis OS X builders understand nodejs now, so I would accept a PR to switch back to language: node_js (but please do that in a separate PR).

@nfischer
Copy link
Member

We can't use npm install, we have our own script (see #896)

This produces different results in CI and production (lockfiles are not working there).

Not sure I quite understand. Are you objecting to the use of lockfiles in general, or objecting to our install wrapper?

@DanielRuf
Copy link
Contributor Author

Not sure I quite understand. Are you objecting to the use of lockfiles in general, or objecting to our install wrapper?

The first, this is why npm ci is a bit dangerous (works in our CI - but not in production).

@nfischer
Copy link
Member

The first, this is why npm ci is a bit dangerous (works in our CI - but not in production).

Might consider something like npm/npm#16867 (comment), since lockfiles have so-far caused more trouble than they've been worth.

@DanielRuf
Copy link
Contributor Author

Probably, or use npm install on CI.
At least CI often produces a different dependency tree than the installed package in production as it is often a child dependency of another dependency and below the root lockfile it is only locked to the SemVer range in the package.json files (or better: pinned there to a specific version).

Not yet sure what the best solution for everyone is. It's a bit inconsistent across platforms (dev, prod, CI), tools (yarn, npm version) and repository settings (a lockfile might be useful for contributors and CI to build reproducible builds but many big devs like Sindre and others do not use lockfiles for such reasons afaik).

Also see sindresorhus/ama#479

nfischer pushed a commit that referenced this pull request Nov 29, 2018
We can safely use `node_js` on all three supported platforms on Travis CI and remove the obsolete scripts.

See #908 (comment)
@nfischer
Copy link
Member

nfischer commented Jan 9, 2019

Replaced by #910, #917, and #921

@nfischer nfischer added the chore label Jan 9, 2019
@nfischer nfischer closed this Jan 9, 2019
@DanielRuf DanielRuf deleted the ci/test-nodejs-6-8-10-11 branch March 17, 2019 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants