Skip to content
This repository has been archived by the owner on Jul 9, 2018. It is now read-only.

Remove npm's package-lock.json files #88

Merged
merged 5 commits into from Feb 23, 2018
Merged

Remove npm's package-lock.json files #88

merged 5 commits into from Feb 23, 2018

Conversation

ntwb
Copy link
Member

@ntwb ntwb commented Feb 22, 2018

Lockfiles for apps, but not for packages
Lockfiles for apps, but not for packages. Lockfiles are great for apps where you want a controlled reproducible environment, but for packages this doesn't make much sense. The package-lock.json files in dependencies are ignored, so the lockfile only applies when users run npm install in the package repo. If you use a lockfile for packages, your local dependency tree will not match the dependency tree of users having your package as a dependency. This can potentially cause problems if one of your package dependencies breaks your package in a patch release. The lockfile will prevent you from seeing the problem locally, but it will affect your users.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

LGTM, it's nice to see it removed as it doesn't add any value in this context.

lerna.json Outdated
@@ -1,5 +1,10 @@
{
"lerna": "2.9.0",
"command": {
"bootstrap": {
"npmClientArgs": ["--no-package-lock"]
Copy link
Member

Choose a reason for hiding this comment

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

Do we need it in the case where we have .npmrc file with the same flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure to be honest, but looking at test/integration/lerna-bootstrap.test.js#L53:
test("--npm-client npm -- --no-optional extends durable npmClientArgs", async () => {

If Lerna is just using the npm command then Lerna should honor the contents of the .npmrc file I expect

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in 56185d5

Copy link
Member

Choose a reason for hiding this comment

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

It's still looks good. Seems to use npm as it is :)

@ntwb ntwb merged commit e116f2a into master Feb 23, 2018
@ntwb ntwb deleted the try/remove-lock-files branch February 23, 2018 09:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants