Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Support "apm install --package-lock-only" #814

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

smashwilson
Copy link
Contributor

@smashwilson smashwilson commented Aug 30, 2018

npm install supports a --package-lock-only option. If provided, node_modules/ is left untouched, and npm only creates or updates the package-lock.json file to reflect the current state of package.json. This is useful for making quick edits to dependency ranges without having to do a full install of a large project (like Atom).

Remaining work
  • Accept the --package-lock-only flag.
  • Document it in the command-line usage.
  • Pass --package-lock-only to the npm install command forked for non-packageDependencies.
  • Ensure that packageDependencies are handled correctly.

@smashwilson
Copy link
Contributor Author

Trying this out in atom/atom, it looks like npm is normalizing file: URIs slightly differently:

diff --git a/package.json b/package.json
index cce9107ba..5916b793b 100644
--- a/package.json
+++ b/package.json
@@ -17,7 +17,7 @@
     "@atom/nsfw": "^1.0.18",
     "@atom/source-map-support": "^0.3.4",
     "@atom/watcher": "1.0.8",
-    "about": "file:packages/about",
+    "about": "file:./packages/about",
     "archive-view": "https://www.atom.io/api/packages/archive-view/versions/0.65.1/tarball",
     "async": "0.2.6",
     "atom-dark-syntax": "https://www.atom.io/api/packages/atom-dark-syntax/versions/0.29.1/tarball",
@@ -130,7 +130,7 @@
     "one-dark-syntax": "https://www.atom.io/api/packages/one-dark-syntax/versions/1.8.4/tarball",
     "one-dark-ui": "https://www.atom.io/api/packages/one-dark-ui/versions/1.12.5/tarball",
     "one-light-syntax": "https://www.atom.io/api/packages/one-light-syntax/versions/1.8.4/tarball",
-    "one-light-ui": "file:packages/one-light-ui",
+    "one-light-ui": "file:./packages/one-light-ui",
     "oniguruma": "6.2.1",
     "open-on-github": "https://www.atom.io/api/packages/open-on-github/versions/1.3.1/tarball",
     "package-generator": "https://www.atom.io/api/packages/package-generator/versions/1.3.0/tarball",

@smashwilson
Copy link
Contributor Author

Hacky as hell but it works:

smashwilson @ hubtop ~/src/atom/atom (aw/apm-up=)
$ ~/src/atom/apm/bin/apm install --package-lock-only
Installing modules ✓
smashwilson @ hubtop ~/src/atom/atom (aw/apm-up=)
$ git diff
smashwilson @ hubtop ~/src/atom/atom (aw/apm-up=)
$

@daviwil
Copy link
Contributor

daviwil commented Aug 30, 2018

So what if we instead changed apm to look for file: entries without the .? When I originally wrote that logic in apm I didn't realize that newer npm didn't need the . anymore (I think I got that from the npm docs). This would remove our need for the extra normalization logic.

The only change in our code would be looking for file: instead of file:. (while also excluding file:/ if the path is a fully-qualified file URI). What do you think?

@smashwilson
Copy link
Contributor Author

👍 I've replace the .startsWith() test with a regexp that'll permit file:aaa/bbb and file:./bbb/ccc but not file:/absolute/path.

Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

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

Awesome, looks great!

@smashwilson
Copy link
Contributor Author

Hmm looks like we have some flakes lurking in here. Starting and stopping the express server we use to mock npm.org and atom.io for specs is asynchronous, but not all of the tests treat it as such, which means that the server can randomly fail to start because port 3000 isn't free any more. Maybe I'll tidy that up later if I get the chance 🤔

@smashwilson
Copy link
Contributor Author

Hmm this branch is still failing (consistently, for me) with an EADDRINUSE. I wonder if that's the external symptom from a different failure... ?

Our Jasmine setup makes it really hard to tell which test is actually failing 🤔

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants