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

fix: dont omit license from stored manifests #7475

Merged
merged 1 commit into from
May 6, 2024
Merged

Conversation

lukekarrys
Copy link
Contributor

This has the effect of adding licenses back into the lockfiles.
Based on code in shrinkwrap.js and inventory.js, it appears that
lockfiles are supposed to store the license. It's likely that in
practice this behavior has not been consistent due to fetching
of minifed manifests and packuments.

I also attempted to remove the license code from shrinkwrap but
that caused many more tests to break. Plus I believe this is the
intended behavior, to have licenses in lockfiles based on bug
reports like #7384

This has the effect of adding licenses back into the lockfiles.
Based on code in shrinkwrap.js and inventory.js, it appears that
lockfiles are supposed to store the license. It's likely that in
practice this behavior has not been consistent due to fetching
of minifed manifests and packuments.

I also attempted to remove the license code from shrinkwrap but
that caused many more tests to break. Plus I believe this is the
intended behavior, to have licenses in lockfiles based on bug
reports like #7384
@lukekarrys lukekarrys requested a review from a team as a code owner May 6, 2024 04:30
Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

this is great; having the license more available will make license-checking tools faster and more reliable.

@lukekarrys lukekarrys merged commit effe910 into latest May 6, 2024
23 checks passed
@lukekarrys lukekarrys deleted the lk/lockfile-license branch May 6, 2024 16:38
@github-actions github-actions bot mentioned this pull request May 6, 2024
techknowlogick pushed a commit to go-gitea/gitea that referenced this pull request Jul 11, 2024
Result of `make update-js`. Tested all dependencies. Lockfile diff is
because of npm/cli#7475.
@jonenst
Copy link

jonenst commented Feb 20, 2025

Hi, thanks for the hard work on npm.
Is it possible to generate the new package-lock with the licenses but still keep everything else as is? Most importantly, the resolved versions of dependencies? using npm 10.9.2, When I rm node_modules and package-lock, everything is reresolved so I get new versions. When I npm install and then remove the package-lock but keep the node_modules folder, npm install regenerates package-lock.json without the licenses. When I npm install and then remove package-lock and also remove node_modules/.package-lock then npm install regenerates with the licenses but without the "resolved" and "integrity" lines

Thanks in advance

@wraithgar
Copy link
Member

The reason the integrity and resolved are missing is a known bug and has to do w/ the fact that it is populating from node_modules and not a tarball. Fixing that should get you what you need.

@jonenst
Copy link

jonenst commented Feb 20, 2025

Thanks for your quick reply ! Did I understand correctly that the easiest way to add the licenses without changing anything else is to use an npm version with #6301 fixed and do
npm i to get the current node_modules,
then rm package-lock.json node_modules/.package-lock.json,
and then npm i again to regenerate them from the actual contents of the all the node_modules files

Do you know which version of npm it should be ? Is there a version already that has the licenses fix and doesn't have this bug ? In the meantime a workaround is to do it manually of course.

Thanks in advance

richmahn pushed a commit to unfoldingWord/dcs that referenced this pull request Mar 2, 2025
Result of `make update-js`. Tested all dependencies. Lockfile diff is
because of npm/cli#7475.
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.

4 participants