Skip to content

Run npm install with npm 5.8.0#767

Merged
yamgent merged 1 commit intomasterfrom
update-package-lock-npm5
Mar 14, 2019
Merged

Run npm install with npm 5.8.0#767
yamgent merged 1 commit intomasterfrom
update-package-lock-npm5

Conversation

@luyangkenneth
Copy link
Contributor

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Other, please explain: Change our package-lock.json to a sane state

What is the rationale for this request?
Refer to #756 for the discussion

@luyangkenneth luyangkenneth requested a review from yamgent March 14, 2019 06:46
@luyangkenneth luyangkenneth force-pushed the update-package-lock-npm5 branch from 20ca545 to b37e3c8 Compare March 14, 2019 06:59
@yamgent
Copy link
Member

yamgent commented Mar 14, 2019

As discussed face-to-face:

Running npm install on both Windows and macOS on the same master branch (commit 6119a91) generates a different version of package-lock.json. One of the obvious main difference is on macOS, these lines are added:

diff --git a/package-lock.json b/package-lock.json
index f4a7194..6f2e2dd 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -214,22 +214,6 @@
         "default-require-extensions": "1.0.0"
       }
     },
+    "aproba": {
+      "version": "1.2.0",
+      "resolved": "https://registry.npmjs.org/aproba/-/aproba-1.2.0.tgz",
+      "integrity": "sha512-Y9J6ZjXtoYh8RnXVCMOU/ttDmk1aBjunq9vO0ta5x85WDQiQfUF9sIPBITdbiiIVcBo03Hi3jMxigBtsddlXRw==",
+      "optional": true
+    },
+    "are-we-there-yet": {
+      "version": "1.1.5",
+      "resolved": "https://registry.npmjs.org/are-we-there-yet/-/are-we-there-yet-1.1.5.tgz",
+      "integrity": "sha512-5hYdAkZlcG8tOLujVDTgCT+uPX0VnpAH28gWsLfzpXYm7wP6mp5Q/gYyR7YQ0cKVJcXJnl3j2kpBan13PtQf6w==",
+      "optional": true,
+      "requires": {
+        "delegates": "1.0.0",
+        "readable-stream": "2.3.3"
+      }
+    },
     "argparse": {
       "version": "1.0.9",

Upon tracking down the packages, we found out that this is because, on Windows, fsevents (which uses all these packages) is not installed, so they are not touched by npm:

npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.1.2 (node_modules\fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.1.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"win32","arch":"x64"})

On macOS, these are needed and requires upgrading, so package-lock.json will have these new lines.

Therefore, running on different platforms on the same code/commit does not always guarantee that the same package-lock.json is being generated.

Since these packages are needed by macOS, I think it would be good to take @luyangkenneth's package-lock.json (since Windows will never touch them) instead of the Windows' version, so that developers using macOS can still update package-lock.json whenever they need to add a new package.
These dependencies will remain undisturbed on the Windows side whenever package-lock.json is updated.

@luyangkenneth
Copy link
Contributor Author

Thanks for typing up our discussion @yamgent! 💯

Also kudos @Xenonym for helping to identify the deal with fsevents so quickly during our discussion 🏅

@yamgent yamgent changed the title Run npm install with npm 5.8.0 Run npm install with npm 5.8.0 Mar 14, 2019
@yamgent yamgent merged commit 33a9ab0 into master Mar 14, 2019
@yamgent
Copy link
Member

yamgent commented Mar 14, 2019

I have merged this PR so that everyone can receive the new copy of package-lock.json, and can also help to test the new copy.

However, as @luyangkenneth also mentioned, the industry actually uses bots to maintain package-lock.json, rather than making developers update it on their own, so that these consistency issues won't pop up in the first place. Future developers should explore the feasibility of this option and implement it.

@yamgent yamgent deleted the update-package-lock-npm5 branch March 14, 2019 08:10
@yamgent
Copy link
Member

yamgent commented Mar 14, 2019

Oh, I forgot to credit @Xenonym in the commit message, sorry about that. :P

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.

2 participants