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

Commit package-lock.json #186

Closed
dotlambda opened this issue Jul 30, 2023 · 15 comments
Closed

Commit package-lock.json #186

dotlambda opened this issue Jul 30, 2023 · 15 comments
Labels
enhancement New feature or request fixed in next Fixed in the "next" branch for the next release

Comments

@dotlambda
Copy link

Since this is not a library its package-lock.json should be tracked by git.

@DavidAnson
Copy link
Owner

This is deliberately not done. Because package-lock.json cannot be published, it is only of use at development time – and I would rather let versions float so nightly CI runs can identify problems as early as possible.

https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json

@DavidAnson DavidAnson added the question Further information is requested label Jul 30, 2023
@dotlambda
Copy link
Author

I would rather let versions float so nightly CI runs can identify problems as early as possible.

Can't the CI script delete package-lock.json before running npm ci?

https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json

Those docs recommend committing package-lock.json.

@DavidAnson
Copy link
Owner

I explained why I don't: it provides no benefit and creates some drawbacks (as I see things).

@dotlambda
Copy link
Author

dotlambda commented Jul 30, 2023

It does provide benefits for reproducably building markdownlint-cli2 for use by e.g. Linux distributions.

@DavidAnson
Copy link
Owner

I haven't heard from anyone trying to do that so far. In terms of users who want reproducibility, the Docker container image is far more comprehensive as it locks the version of this package, all dependencies, the Node runtime, all its dependencies, all the way up to the OS itself.

@dotlambda
Copy link
Author

I haven't heard from anyone trying to do that so far.

I would add markdown-cli2 to NixOS if it had a package-lock.json.

In terms of users who want reproducibility, the Docker container image is far more comprehensive as it locks the version of this package, all dependencies, the Node runtime, all its dependencies, all the way up to the OS itself.

But if you use the Dockerfile on two different machines at slightly different times you might get different versions of the dependencies.

@DavidAnson
Copy link
Owner

Interesting! Does NixOS have a history of including Node tools? Can you point me at some examples?

Regarding Docker, I said the container image offers reproducibility. I agree that building the image again from source at a different time may yield different results. But someone can use the same container images everywhere and get identical results.

@dotlambda
Copy link
Author

Interesting! Does NixOS have a history of including Node tools? Can you point me at some examples?

See for example https://github.com/NixOS/nixpkgs/pull/246155/files#diff-8e7402220a1ea5eb3dab0c71855741c7f6468c246b9a19a443a67494e51b2e77 for markdownlint-cli.

@DavidAnson
Copy link
Owner

@dotlambda
Copy link
Author

It looks like markdownlint-cli2 is already available in NixOS?

Indeed, but using an old method for packaging JS things. I was trying to bring it up to date.
See NixOS/nixpkgs#229475.

@DavidAnson
Copy link
Owner

Is https://docs.npmjs.com/cli/v9/configuring-npm/npm-shrinkwrap-json acceptable as an alternative? It seems better suited for the goal of reproduceability anyway. Specifically, while lock applies only at development time and allows run time to float and cause problems, shrinkwrap seems to apply to both scenarios and should therefore provide consistency.

@dotlambda
Copy link
Author

Yes, that also works.

@DavidAnson DavidAnson added enhancement New feature or request fixed in next Fixed in the "next" branch for the next release and removed question Further information is requested labels Aug 1, 2023
@DavidAnson
Copy link
Owner

This change will be part of the next release. I still don't think I believe in package-lock, but shrinkwrap seems to fit with my way of thinking and offer the guarantee you want. Thanks for bringing this up!

DavidAnson added a commit that referenced this issue Aug 26, 2023
…s cross-platform install problems that are difficult to detect and avoid (refs #186).

This reverts commit 0df0bba.
DavidAnson added a commit that referenced this issue Aug 26, 2023
…s cross-platform install problems that are difficult to detect and avoid (refs #186).

This reverts commit 0df0bba.
DavidAnson added a commit that referenced this issue Aug 26, 2023
…s cross-platform install problems that are difficult to detect and avoid (refs #186).

This reverts commit 0df0bba.
DavidAnson added a commit that referenced this issue Aug 26, 2023
…s cross-platform install problems that are difficult to detect and avoid (refs #186).

This reverts commit 0df0bba.
DavidAnson added a commit that referenced this issue Aug 26, 2023
…s cross-platform install problems that are difficult to detect and avoid (refs #186).
@DavidAnson
Copy link
Owner

FYI, I have reverted this change for v0.9.2. Despite spending hours trying to make it work, I could not dependably detect or avoid cross-platform breaks. I will not try this again until I learn of a successful strategy or npm fixes the issues I was hitting. More discussion here: #198. Sorry about that!

@DavidAnson
Copy link
Owner

FYI, I captured my notes here historical purposes and possible future reference: https://gist.github.com/DavidAnson/39b0eed160f7ce481c92e24a651b5d6f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed in next Fixed in the "next" branch for the next release
Projects
None yet
Development

No branches or pull requests

2 participants