Skip to content

optional packages #3184

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

Merged
merged 1 commit into from
Oct 30, 2018
Merged

optional packages #3184

merged 1 commit into from
Oct 30, 2018

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 30, 2018

This PR adds optional packages info which may help improve the build times.
@etpinard

@etpinard etpinard merged commit cde1f10 into master Oct 30, 2018
@etpinard etpinard deleted the optional_packages branch October 30, 2018 14:52
@etpinard
Copy link
Contributor

etpinard commented Nov 5, 2018

I'm thinking about reverting this PR. On [email protected], checking master and npm i give me this diff:

diff --git a/package-lock.json b/package-lock.json
index 9bc5dd983..5a6f79645 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -3907,14 +3907,12 @@
         "balanced-match": {
           "version": "1.0.0",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "brace-expansion": {
           "version": "1.1.11",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
             "balanced-match": "^1.0.0",
             "concat-map": "0.0.1"
@@ -3929,20 +3927,17 @@
         "code-point-at": {
           "version": "1.1.0",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "concat-map": {
           "version": "0.0.1",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "console-control-strings": {
           "version": "1.1.0",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "core-util-is": {
           "version": "1.0.2",
@@ -4059,8 +4054,7 @@
         "inherits": {
           "version": "2.0.3",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "ini": {
           "version": "1.3.5",
@@ -4072,7 +4066,6 @@
           "version": "1.0.0",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
             "number-is-nan": "^1.0.0"
           }
@@ -4087,7 +4080,6 @@
           "version": "3.0.4",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
             "brace-expansion": "^1.1.7"
           }
@@ -4095,14 +4087,12 @@
         "minimist": {
           "version": "0.0.8",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "minipass": {
           "version": "2.2.4",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
             "safe-buffer": "^5.1.1",
             "yallist": "^3.0.0"
@@ -4121,7 +4111,6 @@
           "version": "0.5.1",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
             "minimist": "0.0.8"
           }
@@ -4202,8 +4191,7 @@
         "number-is-nan": {
           "version": "1.0.1",
           "bundled": true,
-          "dev": true,
-          "optional": true
+          "dev": true
         },
         "object-assign": {
           "version": "4.1.1",
@@ -4215,7 +4203,6 @@
           "version": "1.4.0",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
             "wrappy": "1"
           }
@@ -4337,7 +4324,6 @@
           "version": "1.0.2",
           "bundled": true,
           "dev": true,
-          "optional": true,
           "requires": {
             "code-point-at": "^1.0.0",
             "is-fullwidth-code-point": "^1.0.0",

Searching about the problem doesn't point to a solution:

@alexcjohnson
Copy link
Collaborator

At least by line count, that diff is the same as this PR and the same as what I saw in #3211, using [email protected]. So I'm in favor of reverting.

@alexcjohnson
Copy link
Collaborator

haha we've accidentally done this several times already...

As long as we can get to a stable situation where we can all run npm i without changing package-lock we should do that - and it seems like we CAN be stable without the optional lines but we CANNOT with them. So let's revert.

@archmoj
Copy link
Contributor Author

archmoj commented Nov 5, 2018

These two links might be relevant:
npm/npm#17722
dependabot/feedback#197
@etpinard on linux could you please test if by removing node_modules folder and then npm cache clean --force you may get any diff in the lock?

@alexcjohnson
Copy link
Collaborator

Alright, I think we've gotten to a clear OS difference: I'm now on [email protected] and [email protected] - same as @archmoj on linux - and no matter what I do (even rm -rf node_modules and npm cache clean --force before npm i) these particular optional lines get 🔪

@archmoj
Copy link
Contributor Author

archmoj commented Nov 6, 2018

I also tested this on Windows using [email protected] and [email protected]. I cloned master. I removed npm cache and then npm i gives me those optional packages.

@etpinard
Copy link
Contributor

etpinard commented Nov 6, 2018

then npm i gives me those optional packages.

Could you clarify here? npm i on the current master modifies the package-lock.json or not?

@archmoj
Copy link
Contributor Author

archmoj commented Nov 6, 2018

It gives me those optional packages that are already in the file (plus few more). So it does modify the package-lick.json.

@degrootestad
Copy link

I'm currently having this issue, but can only contribute from a npm user context.
My perception is that re-installing from scratch (with cleaned cache) gives me the "optional": true attributes. When running npm i another time removes them ... does this ring any bells with any of you?

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.

4 participants