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(npm): use shrinkwrap instead of lockfile #803

Merged
merged 7 commits into from
Apr 19, 2023
Merged

fix(npm): use shrinkwrap instead of lockfile #803

merged 7 commits into from
Apr 19, 2023

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Apr 19, 2023

🧰 Changes

This renames our package-lock.json file to npm-shrinkwrap.json. npm treats the file almost identically during installations, but the only difference is that the shrinkwrap is actually published to the npm registry, which isn't recommended for JS libraries but is recommended for CLI tools1.

I only recently realized that package-lock.json isn't published to the npm registry, which explains why the dependency mismatch issue in #703 happened. I suppose a downside of this is that if we have a security vulnerability in our dep tree, users are stuck with it?

🧬 QA & Testing

The shrinkwrap file dates back to at least npm@6 so it shouldn't change anything for CLI users that install/run rdme using Node 14+ (see how Node 14 tests continue to pass).

This is only an npm registry change so this has no affect on GitHub Actions users.

The only users I'm worried about are yarn users (we don't even tell people to install via yarn, but still). Once this change has been published to npm on our next channel, I'm going to try installing it via yarn and see what happens.

Footnotes

  1. You can read more about this in the npm docs: https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json#package-lockjson-vs-npm-shrinkwrapjson

Base automatically changed from kanad/bump-deps/2023-04-19 to next April 19, 2023 16:44
@kanadgupta kanadgupta changed the title chore(npm): use shrinkwrap instead of lockfile fix(npm): use shrinkwrap instead of lockfile Apr 19, 2023
@kanadgupta kanadgupta marked this pull request as ready for review April 19, 2023 20:06
@@ -2,3 +2,4 @@ __tests__/__fixtures__/invalid-json/yikes.json
CHANGELOG.md
coverage/
dist/
npm-shrinkwrap.json
Copy link
Member Author

Choose a reason for hiding this comment

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

TIL that prettier ignores package-lock.json by default 🙄

Copy link
Member

Choose a reason for hiding this comment

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

Huh, I swear I thought that NPM published package-lock.json to the registry unless you excluded it in .npmignore but it's definitely not there. https://www.npmjs.com/package/rdme?activeTab=code

If you install new or update existing packages does that recreate package-lock or does it bump the shrinkwrap?

Copy link
Member Author

Choose a reason for hiding this comment

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

It updates the shrinkwrap1! If both files (package-lock and npm-shrinkwrap) are present, the latter takes precedence and the former is ignored: https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json#package-lockjson-vs-npm-shrinkwrapjson

Footnotes

  1. I confirmed this locally!

@kanadgupta kanadgupta merged commit 4ec3fa7 into next Apr 19, 2023
@kanadgupta kanadgupta deleted the shrinkwrap branch April 19, 2023 20:27
kanadgupta pushed a commit that referenced this pull request Apr 19, 2023
## [8.6.2-next.2](v8.6.2-next.1...v8.6.2-next.2) (2023-04-19)

### Bug Fixes

* **npm:** use shrinkwrap instead of lockfile ([#803](#803)) ([4ec3fa7](4ec3fa7))

[skip ci]
@kanadgupta
Copy link
Member Author

Confirmed that this plays nicely with yarn 🎊

CleanShot 2023-04-19 at 15 50 06@2x

kanadgupta pushed a commit that referenced this pull request Apr 19, 2023
## [8.6.2](v8.6.1...v8.6.2) (2023-04-19)

### Bug Fixes

* **deps:** bump `oas` (and others) ([#802](#802)) ([4dbc5f4](4dbc5f4))
* **npm:** generate provenance statements ([6834f75](6834f75))
* **npm:** use shrinkwrap instead of lockfile ([#803](#803)) ([4ec3fa7](4ec3fa7))
* packages permission issue ([f0fb7dc](f0fb7dc))
* **semantic-release:** bump to v21 ([f53c1a4](f53c1a4))

[skip ci]
@domharrington
Copy link
Member

This is weird, i thought npm basically deprecated the shrinkwrap file and preferred the package-lock ever since it came out 🤷‍♂️ but if it works, it works.

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