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

[BUG] Npm not installing expected package version #3411

Open
1 task done
ambroiseRabier opened this issue Jun 13, 2021 · 7 comments
Open
1 task done

[BUG] Npm not installing expected package version #3411

ambroiseRabier opened this issue Jun 13, 2021 · 7 comments
Assignees
Labels
Bug thing that needs fixing Needs Discussion is pending a discussion Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release

Comments

@ambroiseRabier
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

When installing a new package with npm i <package_name>, if the package already exist as sub-dependencie of another package, it will install that version of the package (example 13.5.3). Instead of installing latest version (example 15.4.3). If that is a design choice, I find it counter-intuitive and hard to debug.

Expected Behavior

npm init
npm i @storybook/components
npm i react-syntax-highlighter

Should give me:

{
  "dependencies": {
    "@storybook/components": "^6.2.9",
    "react-syntax-highlighter": "^15.4.3"
  }
}

Doing npm i <package_name> when the package is absent from package.json should install the latest version by default of that package, ignoring smaller versions installed as sub-dependencie of other installed packages.

Steps To Reproduce

npm init
npm i react-syntax-highlighter
{
  "dependencies": {
    "react-syntax-highlighter": "^15.4.3"
  }
}

I have the latest version, hourra !


npm init
npm i @storybook/components
npm i react-syntax-highlighter
{
  "dependencies": {
    "@storybook/components": "^6.2.9",
    "react-syntax-highlighter": "^13.5.3"
  }
}

Why do I have 13.5.3 version ? It seem to be the same version as the one used by @storybook/components internally.

Environment

  • OS: Window 10
  • Node: v14.16.1
  • npm: 7.10.0
@ambroiseRabier ambroiseRabier added Bug thing that needs fixing Needs Triage needs review for next steps Release 7.x work is associated with a specific npm 7 release labels Jun 13, 2021
@ljharb
Copy link
Collaborator

ljharb commented Jun 13, 2021

Presumably because storybook/components has a peer dep on v13 of react-syntax-highlighter, so that’s the only way the two can possibly be used together (just a guess; haven’t verified)

@ambroiseRabier
Copy link
Author

@ljharb there is a dependencie towards "react-syntax-highlighter": "^13.5.3"

Content of the package.json of @storybook/components:

  "dependencies": {
    "@popperjs/core": "^2.6.0",
    "@storybook/client-logger": "6.2.9",
    "@storybook/csf": "0.0.1",
    "@storybook/theming": "6.2.9",
    "@types/color-convert": "^2.0.0",
    "@types/overlayscrollbars": "^1.12.0",
    "@types/react-syntax-highlighter": "11.0.5",
    "color-convert": "^2.0.1",
    "core-js": "^3.8.2",
    "fast-deep-equal": "^3.1.3",
    "global": "^4.4.0",
    "lodash": "^4.17.20",
    "markdown-to-jsx": "^7.1.0",
    "memoizerific": "^1.11.3",
    "overlayscrollbars": "^1.13.1",
    "polished": "^4.0.5",
    "prop-types": "^15.7.2",
    "react-colorful": "^5.0.1",
    "react-popper-tooltip": "^3.1.1",
    "react-syntax-highlighter": "^13.5.3",
    "react-textarea-autosize": "^8.3.0",
    "regenerator-runtime": "^0.13.7",
    "ts-dedent": "^2.0.0",
    "util-deprecate": "^1.0.2"
  },
  "devDependencies": {
    "css": "^3.0.0",
    "jest": "^26.6.3"
  },
  "peerDependencies": {
    "react": "^16.8.0 || ^17.0.0",
    "react-dom": "^16.8.0 || ^17.0.0"
  },

If NPM choose to take the version of a sub-dependencie instead of the latest when doing npm install, to maximise compatibility while avoiding duplicating packages. I should at least be notified ! And given the choice to have two version of the same package in my node_modules.

@ljharb
Copy link
Collaborator

ljharb commented Jun 13, 2021

Ah, i see what happened. You installed storybook/components first, and the second install command didn’t specify @latest or anything else - so npm prefers what’s already present.

If you’d included a version specifier, then npm would have installed that. In other words, if you want latest, you’ve always had to add “@latest” manually to the install command.

@ambroiseRabier
Copy link
Author

ambroiseRabier commented Jun 13, 2021

@ljharb That is indeed what happened, but isn't that counter intuitive for the user ? When the user want to install a package that he saw on github, he does npm i <my_package>, the doc on github will be for the latest version of the package by default. Meaning that the user can silently get an outdated package, that is not synchronized with the current documentation.

When installing a new package, we most of the time get the latest version (no?), meaning that the user won't expect getting an outdated package. Also, we can't expect someone to know of all the sub dependencies of the installed packages.

I think a warning message would be helpful, I am probably not the only one being throw off by this behavior, the warning could be something like:

"Warning: Installing version 13.5.3 of package react-syntax-highlighter that is already a dependencie of @storybook/components package. Instead of latest version 15.4.3. To install latest version do npm i react-syntax-highlighter@latest"

And why does NPM prefer a package already present ? Is it only to save space ?

@ljharb
Copy link
Collaborator

ljharb commented Jun 13, 2021

To minimize conflicts and duplication, i assume. You might already implicitly be depending on v13 of that package, and installing v15 unintentionally might break things.

@wraithgar
Copy link
Member

In light of #3494 pointing folks to npm install I think this should be characterized as a bug. npm update for sure should be doing this, i.e. only updating within the confines of what everything in the package tree is allowing.

The install docs do say:

In most cases, this will install the version of the modules tagged as latest on the npm registry.

and I am not sure what cases shouldn't do this. I'm pretty sure "some other subdependency is requiring something a semver major away from latest" isn't one of them though.

We'll probably need to discuss this a little internally just to be sure we're not missing something.

@wraithgar wraithgar added Priority 1 high priority issue Needs Discussion is pending a discussion Priority 2 secondary priority issue and removed Needs Triage needs review for next steps Priority 1 high priority issue labels Jul 1, 2021
@isaacs
Copy link
Contributor

isaacs commented Jul 2, 2021

I'd definitely call this a bug. There's no reason why it couldn't install the latest one, it's just a side effect of the "can we avoid having to replace the current thing because it satisfies the dep" coupled with the way that we install new packages by adding a new dependency to the root project on <pkg name>@*. Since 13.5.2 satisfies the * range, it skips the REPLACE and does a KEEP instead.

What it ought to do is note that the parent dep is an explicit request (which is info we have at the time, but aren't using), and clobber it even though it doesn't "technically" have to based on the spec it's working with at the time, unless --prefer-dedupe is set.

The only case where it maybe shouldn't do that, I think, is if the existing dep is held in place by a peerDep from another package. Ie, you install a which has a peerDep on b@1, and b@latest is currently 2.0.0. You do npm i b, should it just add a dependency on b@1, or fail with an ERESOLVE? If we do "use the existing dep if you can when --prefer-dedupe is set, then peerDeps will retain the current behavior (use the existing dep) since peerDeps are always effectively installed in preferDedupe mode.

I'll get a test for this in the Arborist.buildIdealTree refactor I'm working on now, and fix it there.

isaacs added a commit to npm/arborist that referenced this issue Jul 2, 2021
Also, this includes the fix for npm/cli#3411
isaacs added a commit to npm/arborist that referenced this issue Jul 20, 2021
Also, this includes the fix for npm/cli#3411
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Discussion is pending a discussion Priority 2 secondary priority issue Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

No branches or pull requests

4 participants