npm icon indicating copy to clipboard operation
npm copied to clipboard

Updating from `7.0.10` to `7.1.0` breaks `npm install` when running on Node 12 or 14 with NPM `6.X`

Open amclin opened this issue 3 years ago • 12 comments

7.10.0 is a breaking change with the introduction of NPM 7 as a dependency

My project's unit tests run npm install using Node's child_process.spawn

Bumping from 7.0.10 to 7.1.0 or 7.1.1 causes the tests to fail, because the Exit Code for npm install suddenly is 1 instead of 0 https://github.com/amclin/react-project-boilerplate/pull/679

I've narrowed this down by specifying explicit versions of @semantic-release/npm in my package.json with no other changes in my repo. Something about adding NPM 7 as a depedency causes problems when the project is using an older version of NPM as provided by the filesystem (eg, NPM 6.X with Node 14)

My project is designed to be run on multiple versions of Node. Forcing NPM 7 would break compatibility with Node LTS versions 12 and 14, forcing everyone to Node 15. Most people use the version of NPM that comes with Node.

This all seems to work fine when running my project in Node 15 with NPM 7.7. But running my project on Node 12 or Node 14 with NPM 6.x is now failing.

amclin avatar Apr 09 '21 23:04 amclin

I'm sorry you are having issues. NPM 7 should work on node 10 and newer. What is the output of npm install that suddenly returns exitcode 1? Did you try removing node_modules before running npm install, does that help?

danez avatar Apr 10 '21 07:04 danez

this unexpected change from NPM 6 to NPM 7 in a minor version has broken the C/I of my project, which expects NPM 6. the C/I environments are updating to NPM 7 on the fly due to this change, introducing errors.

nlundquist avatar Apr 13 '21 17:04 nlundquist

@danez I've created an example repo stripped down to the bare functionality, with steps to reproduce: https://github.com/amclin/poc-semantic-release-npm-error

Steps to Reproduce

  1. Use Node 12 or Node 14, with default NPM (6.x) provided by that version of Node
  2. Clone this repo
  3. run npm install
  4. run npm start
  5. remove the directory created previously:
    • rm -rf example
  6. Update to @semantic-release/npm v7.1.0:
  7. Run npm start

Error:

npm WARN deprecated [email protected]: Legacy versions of mkdirp are no longer supported. Please update to mkdirp 1.x. (Note that the API surface has changed to use Promises in 1.x.)

added 3 packages, and audited 4 packages in 719ms

3 low severity vulnerabilities

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.
(node:83494) UnhandledPromiseRejectionWarning: Error: Failed to install dependencies using npm install --save-dev jest-coverage-badges
    at ChildProcess.<anonymous> (/Users/amclin/Box Sync/git/poc-semantic-release-npm-error/index.js:39:14)
    at ChildProcess.emit (events.js:314:20)
    at maybeClose (internal/child_process.js:1022:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:287:5)
(node:83494) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:83494) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Specific collision

I've stripped down the child process npm install to one single package that fails jest-coverage-badges.

  • ✅ When the container project has @semantic-release/[email protected] then the subprocess npm install --save-dev jest-coverage-badges works.
  • ❌ When the container project has @semantic-release/[email protected] then the subprocess npm install --save-dev jest-coverage-badges fails.

amclin avatar Apr 13 '21 17:04 amclin

Thanks for the repo. When your script runs npm it is using the npm that is installed in node_modules, which happened to be version 6 and now is version 7 in the latest version of @semantic-release/npm. I can see that this error is triggered now with the update but your script shouldn't be using the version of npm that semantic-release installs.

So I see a couple of options:

  • Do not run the index script via npm but instead directly node ./index.js. This then works because npm adds ./node_modules/.bin to $PATH, so not running the script via npm means it will pick up the system npm. (I guess this is not really an option)
  • Use the flag --no-audit for npm install. For some reason npm install in version 7 fails if executed via npm 6 run-script and there are security issues found.
  • When you call spawn change $PATH in the environment variables and remove ./node_modules/.bin/npm (not really a nice solution though)
  • hardcode the path to the npm binary you want to use, so instead of simply calling npm in your script call /usr/local/node/bin/npm or wherever the system npm binary is located.
  • Add npm as dependency to your project in the version you would like to use.

I hope this helps.

danez avatar Apr 13 '21 18:04 danez

Thanks @danez, I can confirm --no-audit is a workaround for the issue in my project, and I'm determining whether that's the best course of action.

amclin avatar Apr 13 '21 21:04 amclin

another option to consider is to run semantic-release using npx rather than installing it as a local dependency. that way, the npm dependency should not conflict with your other usage of npm in your project.

travi avatar Apr 14 '21 01:04 travi

@travi I'm already doing that. But the semantic-release plugins aren't included via that method.

amclin avatar Apr 14 '21 14:04 amclin

The upgrade to npm 7 should have been a breaking change. 😢

christianmemije avatar May 03 '21 09:05 christianmemije

The upgrade to npm 7 should have been a breaking change. 😢

Not only that, it shouldn't happen at all as long as NPM 7 is not part of the latest Node LTS version.

pfeileon avatar Aug 18 '21 14:08 pfeileon

My project is designed to be run on multiple versions of Node. Forcing NPM 7 would break compatibility with Node LTS versions 12 and 14, forcing everyone to Node 15. Most people use the version of NPM that comes with Node.

This is not true as @danez pointed out.

We do have a thorough test suite which we run against node 10. We upgraded because they all tests passed. If there is a breaking change we didn't caught then the tests are incomplete and we'd appreciate a pull request that adds a test which fails with the current code.

All other problems that are mentioned here are side effects due to a bad OS setup. Please follow Daniel's checklist above: https://github.com/semantic-release/npm/issues/357#issuecomment-818961177

gr2m avatar Aug 18 '21 18:08 gr2m

When you have sub-projects and execute a complex npm script e.g. cd projects/demo && npm run build from your root folder and npm run build executes npx ../whatever on a windows system, you get the error message:

'..' is not recognized as an internal or external command, operable program or batch file.

Your test suite just might not be thorough enough...

The only workable solution atm is to include @semantic-release/[email protected] as a devDependency in the root package.json.

pfeileon avatar Aug 19 '21 11:08 pfeileon

Your test suite just might not be thorough enough...

then fix it, that's how independent open source projects work. We can facilitate contributions as our time allows, but we cannot fix all the edge cases folks might potentially run into ourselves. And there will always be edge cases that we decide not to handle at all because it increases the overall complexity and of the project, and therefore decreases the maintainability.

gr2m avatar Aug 19 '21 18:08 gr2m