Skip to content

refactor: Convert shrinkwrap extractor tool to package-lock - #1551

Open
d3xter666 wants to merge 11 commits into
mainfrom
refactor/shrinkwrap-to-lockfile-convertion
Open

refactor: Convert shrinkwrap extractor tool to package-lock#1551
d3xter666 wants to merge 11 commits into
mainfrom
refactor/shrinkwrap-to-lockfile-convertion

Conversation

@d3xter666

@d3xter666 d3xter666 commented Aug 27, 2026

Copy link
Copy Markdown
Member

JIRA: CPOUI5FOUNDATION-1283

Shrinkwrap is being deprecated (npm/cli#9262) in node, but we want to keep and guard the dependencies that we ship with the UI5 CLI.

We have decided to go with NPM's packge.json bundeDependencies property

bundleDependencies has its own issues when working with monorepos. The core issue is that it simply bundles anything that's within that package's node_modules/ folder and does not consider any hoisted packages that monorepos naturally have.

With this context in mind, we have decided to leverage our internal tool shrinkwrap-extractor and adjust it to fit for the bundleDependencies case.

Here's the expected workflow:

  • Rename shrinkwrap-extractor to something more meaningful in the new context. Proposal: lockfile-extractor
  • Keep the existing CI publish flow (https://github.com/UI5/cli/blob/main/.github/workflows/release-please.yml#L134-L144) and run lockfile-extractor for packages/cli. This will result in package-lock.json file extraction suitable just for the @ui5/cli package.
  • Move packages/cli out of the scope of the monorepo by copying its files in a $TEMP dir.
  • Within the $TEMP dir, remove any non relevant dependencies (devDependencies) from package.json
  • Run npm ci, so that dependencies are installed with the exact same versions that are defined within the generated earlier package-lock.json
  • Run npm pack and npm publish, so that the generated tarball taht includes also the bundledependencies gets published on NPM

Note: This PR simply renames the shrinkwrap-extractor to lockfile-extractor. The only real change is the handling of publish GH Action: https://github.com/UI5/cli/pull/1551/changes#diff-2c84033033d49186c63e6adcd705f63b11ae6814cd76c152c9c486d389fbccf3

@d3xter666
d3xter666 marked this pull request as draft August 27, 2026 14:59
@d3xter666
d3xter666 force-pushed the refactor/shrinkwrap-to-lockfile-convertion branch 3 times, most recently from ab9b808 to 96c7db8 Compare August 27, 2026 15:30
@@ -0,0 +1,340 @@
import {readFile, mkdtemp, writeFile, rm} from "node:fs/promises";

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is actually renamed internal/shrinkwrap-extractor/lib/convertPackageLockToShrinkwrap.js. I don't know why GitHub splits them this way

@d3xter666
d3xter666 force-pushed the refactor/shrinkwrap-to-lockfile-convertion branch from 96c7db8 to 3ca822d Compare August 28, 2026 09:09
@d3xter666
d3xter666 marked this pull request as ready for review August 28, 2026 11:01
@d3xter666
d3xter666 requested a review from a team August 28, 2026 11:01
@d3xter666
d3xter666 force-pushed the refactor/shrinkwrap-to-lockfile-convertion branch 2 times, most recently from 252206e to 9d8b208 Compare August 31, 2026 12:14
Comment thread .github/workflows/release-please.yml Outdated
Comment on lines +151 to +157
# Strip devDependencies so npm ci only installs production deps matching the lock file
node -e "
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8'));
delete pkg.devDependencies;
fs.writeFileSync('package.json', JSON.stringify(pkg, null, '\t'));
"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this explicit devDependencies strip since the lockfile-extractor provides only the prod dependencies. Any npm ci (even npm ci --omit=dev) call wil fail if we don't do this.

@RandomByte

Copy link
Copy Markdown
Member

I assume we will still need #1228? Since the renaming will likely cause some conflicts, which one would you like to get in first?

@d3xter666

d3xter666 commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

I assume we will still need #1228? Since the renaming will likely cause some conflicts, which one would you like to get in first?

Thanks!
I just rebased from main and pushed to resolve conflicts in #1228 and the PR simply closed

@d3xter666
d3xter666 force-pushed the refactor/shrinkwrap-to-lockfile-convertion branch from 7e72929 to d4b3b7b Compare September 3, 2026 11:52
@@ -0,0 +1,85 @@
name: Bundle and Publish @ui5/cli

@d3xter666 d3xter666 Sep 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an actual copy of shrinkwrap/lockfile-extractor run + package deployment to NPM from release-please.yaml

Now it has a dry run flag, so we can integrate this flow into our current CI environment. This way, we can ensure that every merge in the main branch won't break the release process.

@d3xter666
d3xter666 force-pushed the refactor/shrinkwrap-to-lockfile-convertion branch 4 times, most recently from 854d15f to 5bbcf74 Compare September 3, 2026 12:32

@RandomByte RandomByte left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still think we might not handle all cases of dependency hoisting correctly, as the added test in #1569 was supposed to show. At the same time, I don't see how this process would catch such problems. Therefore I'm hesitant to approve this without any planned follow-up work to address those concerns.

npm ci will install packages as specified in the generated package-lock.json. I believe it does not validate whether hoisted dependencies match with the nested project's requirements (i.e. whether the hoisted dependency's version matches the specified range).

For npm publish I believe that it would pack whatever is provided in node_modules, again without further validation whether that matches with the dependency's requirements.

I might be wrong on both points here, and I would appreciate to get some well founded reassurance if that is the case. Otherwise I fear that the package-lock.json we "extract" would silently lead to incorrect dependency versions getting installed. Note that this can not be tested at runtime. Only by validating the dependency tree.

Comment thread .github/workflows/bundle-and-publish-cli.yml Outdated
Comment thread .github/workflows/bundle-and-publish-cli.yml
Comment thread .github/workflows/bundle-and-publish-cli.yml Outdated
Comment thread .github/workflows/github-ci.yml Outdated
@d3xter666
d3xter666 requested a review from RandomByte September 4, 2026 12:19
Comment thread internal/lockfile-extractor/package.json Outdated
Comment thread internal/lockfile-extractor/LICENSES/Apache-2.0.txt
@d3xter666
d3xter666 force-pushed the refactor/shrinkwrap-to-lockfile-convertion branch from a4874fb to 0f1d300 Compare September 4, 2026 13:51
Comment thread package-lock.json Outdated
d3xter666 and others added 6 commits September 4, 2026 18:09
Migrates from the deprecated npm-shrinkwrap.json (npm/cli#9262) to
bundleDependencies: true. The lockfile-extractor now generates a
standalone package-lock.json; the package is then copied outside the
workspace, installed via npm ci, and packed via npm pack to bundle all
production node_modules.
Co-authored-by: Merlin Beutlberger <m.beutlberger@sap.com>
Co-authored-by: Merlin Beutlberger <m.beutlberger@sap.com>
This license file has been left here by accident. We now manage those licenses centrally
The LICENSES/Apache-2.0.txt file is required per-package: the REUSE compliance
workflow runs with --root <package> lint, so each package needs its own LICENSES/
directory. Restores the file that was incorrectly removed.
@d3xter666
d3xter666 force-pushed the refactor/shrinkwrap-to-lockfile-convertion branch from d480b7d to aaff26a Compare September 4, 2026 15:11
@d3xter666

Copy link
Copy Markdown
Member Author

After rebasing this PR onto main, the generated lockfile became inconsistent with package.json: the hoisted chalk@5.6.2 incorrectly occupies chalk, although @ui5/cli requires chalk@6.0.0. As a result, installing the extracted package with npm ci fails because the lockfile is considered out of sync.

This is the dependency-hoisting edge case addressed by #1569.

I have also prepared a48a15e that gives direct dependencies of the new root package priority in the root node_modules. This should unblock us for now and give us time to evaluate the more general solution in #1569. I would also be fine with dropping this commit in favor of the end-to-end solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants