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] dependency loses transitive dependency #3062

Closed
orangecms opened this issue Apr 11, 2021 · 14 comments
Closed

[BUG] dependency loses transitive dependency #3062

orangecms opened this issue Apr 11, 2021 · 14 comments
Assignees
Labels
Bug thing that needs fixing Priority 1 high priority issue Release 7.x work is associated with a specific npm 7 release

Comments

@orangecms
Copy link

orangecms commented Apr 11, 2021

Note: I am not sure if this may be a duplicate, as there a some hundred issues around dependencies and I am not sure about keywords to narrow down the issue I ran into.

Current Behavior:

When I ran npm update in a project, a dependency (recharts, see below) that introduced the v7 lockfile format edit: should be no issue as per #3062 (comment) did not get (at least) one of its transitive dependencies pulled in. See also the issue filed against upstream: recharts/recharts#2525

Here is the package-lock.json of our project:
https://github.com/orangecms/pslab-desktop/blob/recharts-npm7-breakage/package-lock.json

It should include the dependency math-expression-evaluator, which is a transitive dependency coming from reduce-css-calc; see https://github.com/recharts/recharts/blob/e90a4e1d04cafe130c96316ac381abd0fa8c86d2/package.json#L68

Aside, probably unrelated/irrelevant as per #3062 (comment):
Our dependency recharts switched to v7 just before this patch version release that I got in:
https://github.com/recharts/recharts/blame/e90a4e1d04cafe130c96316ac381abd0fa8c86d2/package-lock.json

Additional note: I had to run npm update --legacy-peer-deps because of a specific depency:

npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! While resolving: pslab@2.7.0
npm ERR! Found: electron@12.0.2
npm ERR! node_modules/electron
npm ERR!   dev electron@"^12.0.2" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer electron@"^9.0.0" from electron-load-balancer@3.0.0
npm ERR! node_modules/electron-load-balancer
npm ERR!   electron-load-balancer@"^3.0.0" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

I cannot tell if this is related.

Expected Behavior:

Dependencies should be fully resolved.

Steps To Reproduce:

Checkout https://github.com/orangecms/pslab-desktop/blob/recharts-npm7-breakage, go back one commit, and run npm update.

Environment:

  • OS: Linux 5.11.11
  • Node: 15.13.0
  • npm: 7.8.0
@orangecms orangecms 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 Apr 11, 2021
@ljharb
Copy link
Collaborator

ljharb commented Apr 11, 2021

package-lock is not published, so the lockfile format of a dependency has no impact on consuming projects.

@orangecms orangecms changed the title [BUG] dependency with npm v7 format loses its dependencies [BUG] dependency loses transitive dependency Apr 11, 2021
@orangecms
Copy link
Author

package-lock is not published, so the lockfile format of a dependency has no impact on consuming projects.

Gotcha and thank you for the quick remark! I've edited the title thusly and a mistake in the report. The dependency :chain: got me a bit confused at some point. Now it should be correctly described. :-)

@orangecms
Copy link
Author

Another note: npm update did not update the dependency recharts in our package.json directly, only in our package-lock. I ran an explicit npm i recharts@latest, but that didn't fix the issue. Now looking closer, package.json wasn't even changed at all.
I hope this helps break down the issue.

@orangecms
Copy link
Author

Even fishier, our commit d273e384c0885cef51599a2f8a13cefd4ee5069f had the transitive dependecy removed. I pretty much doubt that it was edited by hand, but will check back with my fellow devs.

@orangecms
Copy link
Author

Ah, I notice that this made the switch to the v7 package-lock in our project. It could be a migration bug. Sorry for all the addenda; this really is quite a ride. 🎢 🎡

@orangecms
Copy link
Author

Ok so ... I had to dig through the several versions of these dependencies to figure out that the module raising the error wasn't supposed to be installed any longer in the version I still had in node_modules/. The rm-rf-reinstall "fixed" it.

@nlf
Copy link
Contributor

nlf commented Apr 12, 2021

am i understanding correctly that this isn't an issue and the dependency that was removed wasn't a bug?

@orangecms
Copy link
Author

I needed to recap, because this thing is a bit of a 🐇 🕳️ .

It is an issue, and the best I can describe it thus far is that npm was trating node_modules/ wrong somehow. It's supposed to align with package{-lock}.json for npm install, but old stuff was left in the modules directory, which ended up as errors at runtime.
I had the same experience when I had it working again after removal and reinstallation, and then trying to downgrade a dependency. Something is just weird overall.

@nlf
Copy link
Contributor

nlf commented Apr 12, 2021

interesting. let me restate the problem here to make sure i'm understanding exactly what's going on.

in my project A, i have a dependency on package B@1.0.0. B@1.0.0 has a dependency on C@1.0.0. i've already run npm install and have a populated and correct node_modules as well as a package-lock.json.

i now run npm update which installs the semver-compatible (based on the range in my package.json) B@1.1.0 however this release of B no longer depends on C. B itself is updated, but C remains in my node_modules.

does this sound correct? is the extraneous dependency still present in the package-lock.json too, or only the node_modules?

@orangecms
Copy link
Author

Almost! Stop after having node_modules populated and having package-lock.json. I check out a newer commit which created a change in package.json/package-lock.json (the update), and then I run npm install to replay and bring my existing node_modules into the new state. However, the old version of dependency C remains in my node_modules.

@nlf
Copy link
Contributor

nlf commented Apr 12, 2021

ah ha! ok, that helps a lot. thanks for talking that through with me. i'll see what i can do to reproduce this and investigate the bug

@nlf nlf added Priority 1 high priority issue and removed Needs Triage needs review for next steps labels Apr 12, 2021
@darcyclarke darcyclarke added this to the OSS - Sprint 28 milestone Apr 14, 2021
@nlf
Copy link
Contributor

nlf commented Apr 15, 2021

npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! While resolving: pslab@2.7.0
npm ERR! Found: electron@12.0.2
npm ERR! node_modules/electron
npm ERR!   dev electron@"^12.0.2" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer electron@"^9.0.0" from electron-load-balancer@3.0.0
npm ERR! node_modules/electron-load-balancer
npm ERR!   electron-load-balancer@"^3.0.0" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

is telling us that you have electron@^12.0.2 as a direct dependency. you're trying to add electron-load-balancer@3.0.0 as another direct dependency, however that version of electron-load-balancer has a peerDependency on electron@^9.0.0 so you get a peer dependency conflict.

following your steps to reproduce above:

> git clone git@github.com:orangecms/pslab-desktop
> cd pslab-desktop
> git checkout recharts-npm7-breakage
> git checkout HEAD^
> npm update --legacy-peer-deps
> npm ls -a

we get a list of errors about missing and invalid dependencies, after hand checking a few of these they all appear to be peer dependencies that were not installed or were installed at invalid versions due to the --legacy-peer-deps which stops enforcing that peer dependencies are of the correct versions.

as for math-expression-evaluator, from what i can find it is not actually a transitive dependency of reduce-css-calc@2.1.8 which is the version present in the tree.

let's look at those dependencies:

> npm show reduce-css-calc@2.1.8 dependencies
{ 'css-unit-converter': '^1.1.1', 'postcss-value-parser': '^3.3.0' }

> npm show css-unit-converter@^1.1.1 dependencies

> npm show postcss-value-parser@^3.3.0 dependencies

the empty lines for the last two commands tell us that these packages do not have dependencies themselves, so math-expression-evaluator is not and should not be a transitive dependency of reduce-css-calc@2.1.8

that being the case, i'm not sure there's actually a bug here since it looks like what you end up with is a valid package tree aside from the missing/invalid peerDependencies.

@orangecms
Copy link
Author

Yes, that is correct. Maybe part of the story got lost a bit here: We switched from recharts@1.5.0 to recharts@2.0.8 in fossasia/pslab-desktop@d273e38 and with that got the npm v7 lockfile (which is why I thought that could be related). The previous recharts@^1.5.0 depended on reduce-css-calc@1.3.0 (see https://github.com/fossasia/pslab-desktop/commit/d273e384c0885cef51599a2f8a13cefd4ee5069f.patch), which depended on math-expression-evaluator until it was rewritten as reduce-css-calc@2.0.0, which recharts@2.x now depends on, dropping math-expression-evaluator. So for some reason, when I had run npm install (even before the upgrade to Electron 12 that I did later on), I still had some older version of recharts amd/or reduce-css-calc lingering in node_modules.

I'm sorry if this costs you a lot of time to investigate and I really appreciate you looking into it. 🙏 Feel free to close the issue if you think it's not very productive. Thank you!

@nlf
Copy link
Contributor

nlf commented Apr 16, 2021

no need to apologize at all! i'm happy to help.

closing this since it seems ultimately there was no issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Priority 1 high 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