SAP / ui5-tooling

An open and modular toolchain to develop state of the art applications based on the UI5 framework
https://sap.github.io/ui5-tooling
Apache License 2.0
466 stars 71 forks source link

Extraneous packages after npm install #283

Closed HoffmannThomas closed 3 years ago

HoffmannThomas commented 4 years ago

Followup to https://github.com/SAP/ui5-cli/issues/282

Might be related to https://github.com/npm/npm/issues/11189

Potential solution: https://github.com/npm/npm/issues/11189#issuecomment-223770995

Expected Behavior

npm ls passes

Current Behavior

npm ERR! extraneous: ava@2.4.0 ...test\node_modules\@ui5\cli\node_modules\ava
npm ERR! extraneous: coveralls@3.0.9 ...test\node_modules\@ui5\cli\node_modules\coveralls
npm ERR! extraneous: cross-env@6.0.3 ...test\node_modules\@ui5\cli\node_modules\cross-env
npm ERR! extraneous: docdash@1.2.0 ...test\node_modules\@ui5\cli\node_modules\docdash
npm ERR! extraneous: eslint@6.8.0 ...test\node_modules\@ui5\cli\node_modules\eslint
npm ERR! extraneous: eslint-config-google@0.14.0 ...test\node_modules\@ui5\cli\node_modules\eslint-config-google
npm ERR! extraneous: eslint-plugin-jsdoc@15.12.2 ...test\node_modules\@ui5\cli\node_modules\eslint-plugin-jsdoc
npm ERR! extraneous: execa@3.4.0 ...test\node_modules\@ui5\cli\node_modules\execa
npm ERR! extraneous: nyc@14.1.1 ...test\node_modules\@ui5\cli\node_modules\nyc
npm ERR! extraneous: open-cli@5.0.0 ...test\node_modules\@ui5\cli\node_modules\open-cli
npm ERR! extraneous: sinon@7.5.0 ...test\node_modules\@ui5\cli\node_modules\sinon
npm ERR! extraneous: tap-nyan@1.1.0 ...test\node_modules\@ui5\cli\node_modules\tap-nyan
npm ERR! extraneous: tap-xunit@2.4.1 ...test\node_modules\@ui5\cli\node_modules\tap-xunit

Steps to reproduce the issue

  1. npm init empty folder
  2. npm install @ui5/cli --save-dev
  3. npm ls

Context

Affected components (if known)

RandomByte commented 4 years ago

Thank you for digging out that issue on npm/npm! I did a quick test and find this very promising.

From my understanding we can basically remove all devDependencies from the npm-shrinkwrap.json using:

  1. npm install - Installs all dependencies (dev- and non-dev) into node_modules
  2. npm prune --production - Removes all devDependencies from node_modules
  3. npm shrinkwrap - Updates npm-shrinkwrap.json based on content of node_modules

Or even quicker:

  1. npm install --production
  2. npm shrinkwrap

In the first scenario I find it weird that npm shrinkwrap still needs to be executed. npm states in their documentation:

any commands that update node_modules and/or package.json’s dependencies will automatically sync the existing lockfile.

Yet somehow npm prune seems to be an exception to this.

Also, the npm shrinkwrap documentation does not mention anything on updating it based on the node_modules directory content.

Anyways, it seems to work 🤷‍♂

RandomByte commented 4 years ago

So, since we want to have locked devDependencies while working on the UI5 CLI project, we can only remove them from the npm-shrinkwrap.json that is going to be published to the npm registry. I guess we can give this a shot.

During release, after the tests have been executed and the version bump is complete (which creates a commit that should not change the shrinkwrap), we can remove all devDependencies (npm prune --production) and update the shrinkwrap (npm shrinkwrap) right before publishing.

I'll update our internal release script. This will probably be in place for UI5 CLI v2.0.

RandomByte commented 4 years ago

The change got implemented into our release script. We'll see the effect with the next release 👍

RandomByte commented 4 years ago

Okay, so the proposed change was in place when we published @ui5/cli@2.0.0. However, a pretty interesting side-effect caused our CI to produce and publish a corrupt npm-shrinkwrap.json. We were able to publish a corrected 2.0.1 from my local machine shortly after but only now I got around to look into the root cause of the corruption during our CI build.

I found that npm test in our case will cause esm to create a couple of .cache directories within node_modules/. They are not removed by npm prune --production, even though the module containing them is pruned. Because of that, npm shrinkwrap will generate npm-shrinkwrap.json entries like "ava": {} and "esm": {}. When installing from that npm-shrinkwrap.json, npm will throw an error Cannot read property 'match' of undefined.

Our workaround is to execute npm ci again after npm test since that will remove the node_modules directory completely before installing all dependencies again. We will also add a validation step using npm pack and a test install of the generated tar before the final publish.

RandomByte commented 4 years ago

Our workaround is to execute npm ci again after npm test since that will remove the node_modules directory completely before installing all dependencies again. We will also add a validation step using npm pack and a test install of the generated tar before the final publish.

This has been done and the release of v2.0.2 worked like a charm 🎉

❯ npm i @ui5/cli@latest

+ @ui5/cli@2.0.2
added 588 packages from 377 contributors and audited 8277 packages in 7.566s

found 0 vulnerabilities

Thanks again for reporting this issue!