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

[WIP] package-lock.json experiment WIP #325

Closed
wants to merge 4 commits into from

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Sep 13, 2018

This experiment illustrates a possible issue with committing package-lock.json as we had agreed in [1] and [2]:

When updating package-lock.json, npm does not seem to be so smart about checking if any lower-level dependencies should be updated.

The commits in this PR illustrate my observation:

  • Step 1 (9dde220) - commit package-lock.json after npm install - shows some npm audit issues due to old [email protected] as explicitly required by [email protected]
  • Step 2 (91dce43) - npm install cordova-lib@^9.0.0-nightly (just a test) - then npm audit issues continue to show up even though old request version 2.79.0 is no longer needed
  • Step 3 (06bf9d5) - remove committed package-lock.json
  • Step 4 (d4184bf) - commit new package-lock.json after npm install --package-lock-only - now with no more npm audit warnings

After step 2 (91dce43), npm audit gave me the following suggestion:

  • Run npm update request --depth 2 to resolve 5 vulnerabilities

While this suggestion should be able to resolve the warnings, I really find this process to be a bit clumsy and non-intuitive.

[1] https://lists.apache.org/thread.html/7f92561d382f143aaf49e083bbe215dcf95a3f4d8b6e3cbb6089a5f3@%3Cdev.cordova.apache.org%3E
[2] apache/cordova#4

Christopher J. Brody added 4 commits September 13, 2018 12:35
(with node_modules completely removed)

npm audit continues to show warnings due to the following
residual request entry in package-lock.json:

    "request": {
      "version": "2.79.0",
      "resolved": "http://registry.npmjs.org/request/-/request-2.79.0.tgz",
      "integrity": "sha1-Tf5b9r6LjNw3/Pk+BLZVd3InEN4=",
      "requires": { ... },
      "dependencies": { ... }
      }
@dpogue
Copy link
Member

dpogue commented Sep 13, 2018

Our process at work for updating package-lock.json is to delete the whole file and the node_modules folder and then run npm install again to regenerate it with the latest versions of everything.

Note that package-lock.json is only used for the local dev environment for the package, it's not used when installing the package as a dependency of another project (as far as I know).

@brodycj
Copy link
Contributor Author

brodycj commented Sep 13, 2018

Our process at work for updating package-lock.json is to delete the whole file and the node_modules folder and then run npm install again to regenerate it with the latest versions of everything.

I was thinking we should do this before each major release, maybe should be more often.

@raphinesse
Copy link
Contributor

Using [email protected], I cannot reproduce Step 4. To get the new version I have to remove node_modules before a npm install.

@raphinesse
Copy link
Contributor

Actually npm is doing exactly what I would expect. Mind you that the goal of package-lock.json is to minimize any changes in the (transitive) dependencies that were not explicitly requested.

  • In Step 1, request is resolved to 2.79.0 since that version is pinned in cordova-lib@8 and valid for the range ^2.74.0 given in insight, which we depend on. This is then saved to package-lock.json.
  • In Step 2, when we update cordova-lib to a version that does not depend on request anymore, our resolved dependency to request still satisfies what is requested by insight. So it stays the way it is. Again, this is what the lock file is for. We did not request any updates to be made apart from cordova-lib. Instead, npm tells us how to perform the minimal update necessary to solve the audit issues it found (either npm audit fix or npm update request --depth 2). I think that's a good thing.

Of course, if you want to resolve the whole dependency tree again and do as many updates as possible, then deleting the lock file and regenerating it is a valid approach. Granted, it's a little bit extra work, but for that you only update stuff when you explicitly want to.

I just recently spent a few hours going half-crazy when, unbeknownst to me, jasmine was updated from 3.1.0 to 3.2.0 and suddenly a bunch of tests in cordova-lib that worked fine previously just started to fail. That's exactly the thing that package-lock.json tries to avoid.

@brodycj brodycj closed this Mar 24, 2019
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