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

Support npm 7 #128

Merged
merged 6 commits into from
Oct 23, 2020
Merged

Support npm 7 #128

merged 6 commits into from
Oct 23, 2020

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Oct 17, 2020

Closes #108.

Instead use npm_config_* which works as far back as npm > 0.1.7.
It translates command line flags to environment variables, where:

--foo becomes npm_config_foo='true'
--no-foo becomes npm_config_foo=''
--foo=false becomes npm_config_foo=''
bin.js Outdated Show resolved Hide resolved
test/skip-test.js Outdated Show resolved Hide resolved
util.js Outdated Show resolved Hide resolved
@vweevers

This comment has been minimized.

bin.js Outdated Show resolved Hide resolved
@vweevers
Copy link
Member Author

Wrote npm-script-context to help debug the various cases. Oddities in npm 7:

  1. When installing a github dependency (e.g. github:foo/bar), its install scripts run twice: after the repository is cloned to npm's global cache (cacache) and after copying from cache to local node_modules. In either case, there's no .git directory that we could detect.
  2. When installing a transitive dependency, the output of its install script is swallowed regardless of --loglevel
  3. env.npm_package_from is never defined, I tried various scenarios. I do however see npm_package_resolved=git+ssh://[email protected]/foo/bar.git#abc (but not in the cache case of 1)

I.e. when running `npm install` in the working directory of a
project that uses prebuild-install, you must now run a more
explicit `npm install --build-from-source`.

This is necessary to support npm 7, where we do not have enough
information to automatically determine it and would erroneously
skip downloads on normal installs.
@vweevers vweevers changed the title Prepare for npm 7 Support npm 7 Oct 18, 2020
@vweevers vweevers added the semver-major Changes that break backward compatibility label Oct 18, 2020
@vweevers vweevers marked this pull request as ready for review October 18, 2020 14:21
@lovell
Copy link
Member

lovell commented Oct 20, 2020

Thank you for tackling this Vincent.

https://github.com/npm/cli/blob/latest/CHANGELOG.md#all-lifecycle-scripts suggests we should expect npm_package_resolved (there's no mention of npm_package_from), which I think on its own is enough to recreate the existing _from based logic. If it's not being made available in some cases then that could be a bug.

@vweevers
Copy link
Member Author

which I think on its own is enough to recreate the existing _from based logic.

Aye, that's what I ended up doing in this PR. With npm_package_resolved we can detect a git repository (npm i user/foo or npm i git+file://...) but we cannot detect a standalone install (cd foo && npm i). I removed that behavior (see last commit).

@lovell
Copy link
Member

lovell commented Oct 20, 2020

The lack of standalone install might be a bit annoying for local development e.g. can no longer just run npm i to force compilation, but I agree that the default behaviour should be to download rather than compile.

A possible workaround for local development might be to patch-bump the package.json version in a commit straight after running npm publish to ensure prebuilt binaries aren't found (although this would prevent the use of prebuild-ci).

@vweevers
Copy link
Member Author

@lovell is that a blocker for you, and why is npm i --build-from-source not sufficient?

@lovell
Copy link
Member

lovell commented Oct 23, 2020

@vweevers No blocker, it's only a small change to my development workflow. The benefits to end users far outweigh this, which is where our focus should lie.

.travis.yml Show resolved Hide resolved
rc.js Show resolved Hide resolved
util.js Show resolved Hide resolved
@vweevers
Copy link
Member Author

Not squashing this, because the individual commit descriptions (especially ddb8f69) are useful.

@vweevers vweevers merged commit f9ed67e into master Oct 23, 2020
@vweevers vweevers deleted the npm7 branch October 23, 2020 10:28
@vweevers
Copy link
Member Author

I forgot @ralphtheninja released a new version in the mean time, I should have rebased, commits are now out of order. Sorry.

@vweevers
Copy link
Member Author

I think in this case a rebase and force push to master is acceptable. Otherwise people will be confused why commits with breaking changes are listed before 5.3.6 (although we have a changelog now but not all folks read that).

Do you agree @lovell @ralphtheninja?

@vweevers
Copy link
Member Author

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Changes that break backward compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepare for npm change to _ fields
2 participants