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

npm shrinkwrap fails >= 2.5.0 #5687

Closed
akoskm opened this Issue Mar 26, 2016 · 20 comments

Comments

Projects
None yet
8 participants
@akoskm

akoskm commented Mar 26, 2016

I'm encountering the following error when trying to do npm shrinkwrap on a package.json which contains eslint 2.5.0 or 2.5.1:

$ npm shrinkwrap
npm ERR! Linux 3.13.0-83-generic
npm ERR! argv "/usr/bin/nodejs" "/usr/bin/npm" "shrinkwrap"
npm ERR! node v4.4.1
npm ERR! npm  v2.14.20

npm ERR! Problems were encountered
npm ERR! Please correct and try again.
npm ERR! missing: through@^2.3.6, required by inquirer@0.12.0
npm ERR! missing: esprima@^2.6.0, required by js-yaml@3.5.5
npm ERR! 
npm ERR! If you need help, you may report this error at:
npm ERR!     <https://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /home/akoskm/Projects/project/npm-debug.log

Reverting eslint to 2.4.0 made the error disappear.

@eslintbot eslintbot added the triage label Mar 26, 2016

@platinumazure

This comment has been minimized.

Show comment
Hide comment
@platinumazure

platinumazure Mar 26, 2016

Member

Probably related to our other bundledDependencies issues.

Member

platinumazure commented Mar 26, 2016

Probably related to our other bundledDependencies issues.

@akoskm

This comment has been minimized.

Show comment
Hide comment
@akoskm

akoskm Mar 26, 2016

Yeah, I looked at the history of package.json and was wondering if that's causing it.

akoskm commented Mar 26, 2016

Yeah, I looked at the history of package.json and was wondering if that's causing it.

@platinumazure

This comment has been minimized.

Show comment
Hide comment
@platinumazure

platinumazure Mar 26, 2016

Member

@akoskm Any chance you can try with npm@3? I'm just curious to see if that makes a difference. (We still intend to fix this issue regardless, but sometimes npm@3's deduping can mask the issue and maybe that would generate your shrinkwrap successfully.)

Member

platinumazure commented Mar 26, 2016

@akoskm Any chance you can try with npm@3? I'm just curious to see if that makes a difference. (We still intend to fix this issue regardless, but sometimes npm@3's deduping can mask the issue and maybe that would generate your shrinkwrap successfully.)

@akoskm

This comment has been minimized.

Show comment
Hide comment
@akoskm

akoskm Mar 26, 2016

Sure, I'll let you know once I tested (probably tomorrow).

akoskm commented Mar 26, 2016

Sure, I'll let you know once I tested (probably tomorrow).

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Mar 26, 2016

Member

We were running into shrinkwrap issues yesterday. Haven't had the time to investigate yet.

Member

ilyavolodin commented Mar 26, 2016

We were running into shrinkwrap issues yesterday. Haven't had the time to investigate yet.

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Mar 26, 2016

Member

Fair warning, this is my first time working with some of this stuff, so please correct me if I'm wrong :)

This only seems to affect npm@2 (I am able to replicate the original poster's error with npm@2, but npm@3 seems to be working as expected), so it seems like @platinumazure's comment above is probably what's going on.

@ilyavolodin shared this npm/npm#8396, which seems to be the relevant workaround. Testing locally it looks to me like adding through and esprima to our bundled dependencies fixes the issue. I can make a PR if that seems like the way forward.

Member

kaicataldo commented Mar 26, 2016

Fair warning, this is my first time working with some of this stuff, so please correct me if I'm wrong :)

This only seems to affect npm@2 (I am able to replicate the original poster's error with npm@2, but npm@3 seems to be working as expected), so it seems like @platinumazure's comment above is probably what's going on.

@ilyavolodin shared this npm/npm#8396, which seems to be the relevant workaround. Testing locally it looks to me like adding through and esprima to our bundled dependencies fixes the issue. I can make a PR if that seems like the way forward.

@kaicataldo kaicataldo added accepted and removed evaluating labels Mar 26, 2016

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Mar 26, 2016

Member

@kaicataldo I'm not sure that's the right path forward. I'm not very familiar with all of the npm stuff, but adding dependencies just to satisfy npm seems wrong to me.

Member

ilyavolodin commented Mar 26, 2016

@kaicataldo I'm not sure that's the right path forward. I'm not very familiar with all of the npm stuff, but adding dependencies just to satisfy npm seems wrong to me.

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Mar 26, 2016

Member

@ilyavolodin That's fair. I'll keep digging to see if I can find the underlying cause.

Member

kaicataldo commented Mar 26, 2016

@ilyavolodin That's fair. I'll keep digging to see if I can find the underlying cause.

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Mar 26, 2016

Member

Unfortunately, seems like a known bug: npm/npm#10634 npm/npm#7887. Doesn't sound like it's necessarily going to be fixed anytime soon either (at least as of the time of that posting).

Member

kaicataldo commented Mar 26, 2016

Unfortunately, seems like a known bug: npm/npm#10634 npm/npm#7887. Doesn't sound like it's necessarily going to be fixed anytime soon either (at least as of the time of that posting).

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Mar 26, 2016

Member

The link in my comment above explains what's going on, but the issue seems to be that dev dependencies are not bundled in npm@2 even if they are a sub-dependency of a non-dev dependency. This is only a problem in npm@2 because npm@3 fills in the missing dependencies.

Specifically to this case, through and esprima are listed as dev dependencies for ESLint while also being sub-dependencies of inquirer and js-yaml. Any better ideas than explicitly including through and esprima in our bundled dependencies? I don't like the idea of moving them out of the dev dependencies list in the regular dependencies list because that's not how ESLint consumes them.

Member

kaicataldo commented Mar 26, 2016

The link in my comment above explains what's going on, but the issue seems to be that dev dependencies are not bundled in npm@2 even if they are a sub-dependency of a non-dev dependency. This is only a problem in npm@2 because npm@3 fills in the missing dependencies.

Specifically to this case, through and esprima are listed as dev dependencies for ESLint while also being sub-dependencies of inquirer and js-yaml. Any better ideas than explicitly including through and esprima in our bundled dependencies? I don't like the idea of moving them out of the dev dependencies list in the regular dependencies list because that's not how ESLint consumes them.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Mar 26, 2016

Member

Including them in bundledDependencies might be a reasonable way to go, I think. But not in regular dependencies.

Member

ilyavolodin commented Mar 26, 2016

Including them in bundledDependencies might be a reasonable way to go, I think. But not in regular dependencies.

kaicataldo added a commit that referenced this issue Mar 26, 2016

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Mar 26, 2016

Member

Aware this probably needs more input and discussion from the rest of the @eslint/eslint-team, but made a preliminary PR

Member

kaicataldo commented Mar 26, 2016

Aware this probably needs more input and discussion from the rest of the @eslint/eslint-team, but made a preliminary PR

kaicataldo added a commit to kaicataldo/eslint that referenced this issue Mar 26, 2016

Fix: Fix bundleDeps for npm@2 shrinkwrap (fixes eslint#5687)
Include sub-dependencies that are also dev dependencies in bundled dependencies

kaicataldo added a commit that referenced this issue Mar 26, 2016

Fix: Fix bundleDeps for npm@2 shrinkwrap (fixes #5687)
Include sub-dependencies that are also dev dependencies in bundled dependencies
@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Mar 27, 2016

Member

Good catch identifying the origin of issue @kaicataldo!

Wouldn't budledDependencies get overwritten in every release?

Member

alberto commented Mar 27, 2016

Good catch identifying the origin of issue @kaicataldo!

Wouldn't budledDependencies get overwritten in every release?

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Mar 27, 2016

Member

Also, this doesn't fix the underlying issue, so we should discuss additional measures to avoid this from happening again.

Member

alberto commented Mar 27, 2016

Also, this doesn't fix the underlying issue, so we should discuss additional measures to avoid this from happening again.

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Mar 27, 2016

Member

@alberto From my understanding, the packages in bundledDependencies are bundled when npm publish (or npm pack) is run. Looking at eslint-reslease, it looks like we use a 3rd party package to actually create the bundledDependencies list, and in looking over that package's source, it doesn't look like it accounts for this bug in npm@2.

Realizing this, it seems like my PR actually isn't the right solution, since I think we dynamically generate this list with the build script. Can you confirm, @nzakas? If so, I'll close my PR.

Unless I'm missing something, it seems like the underlying issue is the aforementioned bug. If we want to continue using bundledDependencies in an npm@2 compatible way we could update the build script to account for this bug (i.e. reach out to the 3rd party package author and notify them of the bug and see if they can fix it or just write our own script that does something similar).

Member

kaicataldo commented Mar 27, 2016

@alberto From my understanding, the packages in bundledDependencies are bundled when npm publish (or npm pack) is run. Looking at eslint-reslease, it looks like we use a 3rd party package to actually create the bundledDependencies list, and in looking over that package's source, it doesn't look like it accounts for this bug in npm@2.

Realizing this, it seems like my PR actually isn't the right solution, since I think we dynamically generate this list with the build script. Can you confirm, @nzakas? If so, I'll close my PR.

Unless I'm missing something, it seems like the underlying issue is the aforementioned bug. If we want to continue using bundledDependencies in an npm@2 compatible way we could update the build script to account for this bug (i.e. reach out to the 3rd party package author and notify them of the bug and see if they can fix it or just write our own script that does something similar).

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Mar 27, 2016

Contributor

This is a breaking change in a minor version, so it'd be awesome if this could be fixed soon - I'd love to use the new rules :-/

Contributor

ljharb commented Mar 27, 2016

This is a breaking change in a minor version, so it'd be awesome if this could be fixed soon - I'd love to use the new rules :-/

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 28, 2016

Member

This only occurred because npm was flakey when the package was created. I already updated the release script in #5688, so there shouldn't be any other change necessary besides doing a new release, which I'll do sometime today.

@ljharb this is just a bug, not a breaking change. It's akin to forgetting to include a dependency in package.json. We are doing the best we can to resolve this as quickly as possible, si a bit of patience would be greatly appreciated.

Member

nzakas commented Mar 28, 2016

This only occurred because npm was flakey when the package was created. I already updated the release script in #5688, so there shouldn't be any other change necessary besides doing a new release, which I'll do sometime today.

@ljharb this is just a bug, not a breaking change. It's akin to forgetting to include a dependency in package.json. We are doing the best we can to resolve this as quickly as possible, si a bit of patience would be greatly appreciated.

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Mar 28, 2016

Member

@nzakas are you sure? It looks exactly like the issue mentioned above. I hope you are right :)

Member

alberto commented Mar 28, 2016

@nzakas are you sure? It looks exactly like the issue mentioned above. I hope you are right :)

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Mar 28, 2016

Member

@alberto not I'm not sure. :) I had done a quick test on Saturday that seemed to indicate I was right, but doing another test right now, it's broken again.

We'll just revert to not using bundled dependencies for now and revisit at some later point.

Member

nzakas commented Mar 28, 2016

@alberto not I'm not sure. :) I had done a quick test on Saturday that seemed to indicate I was right, but doing another test right now, it's broken again.

We'll just revert to not using bundled dependencies for now and revisit at some later point.

nzakas added a commit that referenced this issue Mar 28, 2016

@nzakas nzakas closed this in 8749ac5 Mar 28, 2016

nzakas added a commit that referenced this issue Mar 28, 2016

Merge pull request #5708 from eslint/issue5687
Build: Disable bundling dependencies (fixes #5687)
@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Mar 28, 2016

Contributor

@nzakas fair enough, thank you for fixing it!

Contributor

ljharb commented Mar 28, 2016

@nzakas fair enough, thank you for fixing it!

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.