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

dedupe: get deps from shrinkwrap #118

Closed
wants to merge 4 commits into from
Closed

Conversation

larsgw
Copy link
Contributor

@larsgw larsgw commented Dec 13, 2018

@larsgw larsgw requested a review from a team as a code owner December 13, 2018 22:35
@larsgw
Copy link
Contributor Author

larsgw commented Dec 13, 2018

(Breaking) side effects include:

  • installing any package that is in package-lock.json but not in node_modules

It doesn't install packages that are only specified in package.json.

@zkat zkat added the semver:major backwards-incompatible breaking changes label Jan 7, 2019
@aeschright
Copy link
Contributor

We're going to make sure this doesn't have any unintended consequences and goes in the right direction.

Copy link
Contributor

@aeschright aeschright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd like you to redo the test using our standard test script: see scripts/maketest -- you can give it a directory fixture and it will give you the test setup. It'll help us keep things consistent and avoid some older conventions that aren't effective.

@larsgw
Copy link
Contributor Author

larsgw commented Jan 16, 2019

I added the tests, and a doc change now too. I think this change is going to be pretty controversial, seeing how many people already disagree with npm install behavior, fixing incorrect package-lock.json files. The only middle way I can think of, fixing the bug and keeping the behavior, would be to somehow merge all movements in the node_modules tree with all the modules left behind in the shrinkwrap tree, but I don't know if that's feasible.

@zkat zkat force-pushed the release-next branch 2 times, most recently from 6ca2f43 to 6d0cc95 Compare January 18, 2019 19:17
@zkat zkat force-pushed the release-next branch 3 times, most recently from db63b89 to b09bc8c Compare January 23, 2019 18:36
@zkat zkat force-pushed the release-next branch 2 times, most recently from 06cdf5b to f957798 Compare February 20, 2019 20:42
@G-Rath
Copy link
Contributor

G-Rath commented May 14, 2019

What's the status of this? The community issue was closed, so I can't ask on that.

For me as a Windows user, with my expectation of what ddp does, I find it odd that running ddp results in a package-lock.json that will change if I then run npm i.

The core question I have is: Why is npm i able to generate a package-lock.json that includes fsevents without actually installing that package?

@FranklinYu
Copy link

@G-Rath I don’t think that’s an issue. Optional dependency should also be locked, so that when the package is installed on another platform, it won’t change the lock file; this way the lock file converges. In contrary, it doesn’t make sense to remove fsevents when it doesn’t apply to current platform.

@G-Rath
Copy link
Contributor

G-Rath commented May 14, 2019

@FranklinYu For sure.

My question wasn't a challenge at npm i, but as a starting point to try and figure out how ddp should act, as this PR has the breaking side effect of :

installing any package that is in package-lock.json but not in node_modules

The point I was wanting to draw attention to with my question is that npm i doesn't have to install all packages to have them in the package-lock.json, and so why does ddp have to?

While I'm sure there's a good reason, in my eyes that highlights a starting point for identifying where ddp drifts away from i.

@FranklinYu
Copy link

installing any package that is in package-lock.json but not in node_modules

I think that only refers to packages that work for current platform. For example, since fsevents doesn't work in Windows, even if it's in lock file, not in node_modules, it won't be installed by dedupe.

@G-Rath
Copy link
Contributor

G-Rath commented Jun 10, 2019

For example, since fsevents doesn't work in Windows, even if it's in lock file, not in node_modules, it won't be installed by dedupe.

Which is fine, except dedupe removes fsevents from the lock file, which is my whole issue :)

Currently, if I run npm i on any computer, given the same package.json (and technically a complete freeze of time so that no new versions are released), I'll get the same lock file regardless of OS.

Currently, if I run npm ddp on any computer, given the same package.json (and technically a complete freeze of time so that no new versions are released), I'll get a lock file that will differ based on what packages are supported by that OS.

Example:

Given this package.json:

{
  "optionalDependencies": {
    "fsevents": "^2.0.7"
  }
}

Running npm i on Windows, Linux, & OSX gives you the following lock:

{
  "requires": true,
  "lockfileVersion": 1,
  "dependencies": {
    "fsevents": {
      "version": "2.0.7",
      "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.0.7.tgz",
      "integrity": "sha512-a7YT0SV3RB+DjYcppwVDLtn13UQnmg0SWZS7ezZD0UjnLwXmy8Zm21GMVGLaFGimIqcvyMQaOJBrop8MyOp1kQ==",
      "optional": true
    }
  }
}

Running npm ddp on Windows or Linux gives you:

{
  "lockfileVersion": 1
}

Running npm ddp on Mac gives you:

{
  "requires": true,
  "lockfileVersion": 1,
  "dependencies": {
    "fsevents": {
      "version": "2.0.7",
      "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.0.7.tgz",
      "integrity": "sha512-a7YT0SV3RB+DjYcppwVDLtn13UQnmg0SWZS7ezZD0UjnLwXmy8Zm21GMVGLaFGimIqcvyMQaOJBrop8MyOp1kQ==",
      "optional": true
    }
  }
}

Note that nothing in node_modules has changed - the only change happening here is in the package-lock.json (aside from npm i on OSX, which of course installs fsevents)

@FranklinYu
Copy link

Which is fine, except dedupe removes fsevents from the lock file, which is my whole issue :)

Exactly. The whole point of this PR is that dedupe should not remove fsevents.

@G-Rath
Copy link
Contributor

G-Rath commented Jun 10, 2019

The whole point of this PR is that dedupe should not remove fsevents.

Exactly. I've never said otherwise; but it's good we're on the same page 🙂

My question is about the side effect:

installing any package that is in package-lock.json but not in node_modules

I'm interested in why this side effect occurs, given that npm i doesn't have this side effect?

From my understanding, the worst-case bare minimal solution would be to simply run npm i after npm ddp. That is of course highly inefficient, but it's a starting point - from there you can look to strip away unneeded parts from npm i until you're left w/ the minimal code required to "fix" the side effect.

That's a "fix it in post" style approach - which isn't bad; those kind of fixes are often just far more simpler & maintainable than the alternative: try to "fix" the actual root cause of the problem.

This could just be b/c I'm misunderstanding what is meant by "installing any package that is in package-lock.json but not in node_modules", or that just due to the complex nature of npm, that's just how it is, but as I've said: there is at least one way to work around it, that I don't see any major downsides to, given that it seems to give what we all agree is the least astonishing result from running such a command.

Eitherway, I'm just generally interested in understanding more about npm works that means that this is a side effect (w/o spending hours taking it apart line by line*) 😄

*: I mean, I'd love to do this, but sadly don't have the time 😂

@larsgw
Copy link
Contributor Author

larsgw commented Jun 10, 2019

I'm interested in why this side effect occurs, given that npm i doesn't have this side effect?

From what I remember (it's been a while), npm dedupe works like this:

  1. Read node_modules/ tree (like npm ls, and not package-lock.json like npm i)
  2. Perform deduping on the tree
  3. Write the tree to node_modules/ and package-lock.json

Since non-installed optional dependencies have no record in the node_modules/ tree they aren't included in the new package-lock.json. To fix that, I see two possibilities:

  1. Read from package-lock.json instead of node_modules. This has the added side-effect of installing (or trying to install) any packages that are in the package lock, but not in node_modules. This is because only reading the package lock can't tell you certain packages shouldn't be installed, just like only reading node_modules can't tell you if certain packages should be installed. However, that seemed like a smaller problem to me.
  2. What I mentioned above: somehow track the movements of the deduping process and merge those with the package lock, but leave the rest of the process. Way above my pay-grade as a voluntary contributor (and please don't spend time doing that yourself either).

PS: I think I thought of a third option just now, but I can't think of it anymore so I'll come back to this if the thought does.

@G-Rath
Copy link
Contributor

G-Rath commented Jun 10, 2019

Ah interesting - thanks for the explanation :)

Would it be viable to try and leverage the logic that npm i uses to determine if a package should be installed or not?

That could be going in the direction you said not spend time trying to do via 2. :)

I'm also wondering if there is any to know (or get an idea of) all the possible conditions that can result in a package not being installed but being written to package-lock.json.

So far I only know of optionalDependencies, which from what happens w/ fsevents, seem to always be included in the package-lock.json, and so wouldn't be something ddp has to worry about?

I feel like this strengthens the argument that ddp should optimise based on package-lock.json: while it's annoying, if you're going to install a dependency in some situations, you should optimise for that situation, since otherwise isn't it harder/more work to safely determine what packages should be installed?

This has the added side-effect of installing (or trying to install)

To me that sounds like it'll work, depending on how the "trying" is handled; Looking at the error output from doing npm i fsevents on Windows, it's throwing EBADPLATFORM - That to me seems like an exception that'd only really be thrown in a predictable situation (unlike say EBADPERMISSIONS^).

Which I think boils back down to the first part of this comment, about how does npm i determine if a package should be installed, and can that be leveraged here?

*: Luckly, fsevents has no dependencies, and from what I've seen, it's the most common optionalDependencies out there.
^: Might not be an actual exception, but you know what I mean

@FranklinYu
Copy link

I like option 1. Package lock file should be the source of truth. Everyone knows that node_modules is transient and nobody checks it into version control.

@G-Rath
Copy link
Contributor

G-Rath commented Jun 11, 2019

Plus if you do "need" to modify anything in node_modules, you can use patch-package to do so stably.

@FranklinYu
Copy link

sane seems to have switched from fsevents package to fs.watch(). For future testers, we have other examples:

Both depend on fsevents for now.

@jpsfs
Copy link

jpsfs commented May 6, 2020

@FranklinYu any update on this?

@FranklinYu
Copy link

@jpsfs I think you’re mentioning the wrong user. I’m not part of npm team so I can’t merge this even though I want this feature. I haven’t see any npm member involved in discussion by far, so I wouldn’t expect this to happen any time soon.

@darcyclarke
Copy link
Contributor

@larsgw sorry for the delay here. We're going to close this as much of this code has been moved to Arborist (dedupe has been rewritten in v7)

@FranklinYu
Copy link

@darcyclarke Did you imply that NPM v7 won’t have this issue any more?

antongolub pushed a commit to antongolub-forks/npm-cli that referenced this pull request May 18, 2024
🤖 I have created a release *beep* *boop*
---


## [4.0.4](npm/bin-links@v4.0.3...v4.0.4)
(2024-05-04)

### Bug Fixes

*
[`100a4b7`](npm/bin-links@100a4b7)
[npm#117](npm/bin-links#117) linting:
no-unused-vars (@lukekarrys)

### Chores

*
[`e955437`](npm/bin-links@e955437)
[npm#117](npm/bin-links#117) bump
@npmcli/template-oss to 4.22.0 (@lukekarrys)
*
[`b602aca`](npm/bin-links@b602aca)
[npm#117](npm/bin-links#117) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`955cc34`](npm/bin-links@955cc34)
[npm#116](npm/bin-links#116) bump
@npmcli/template-oss from 4.21.3 to 4.21.4 (@dependabot[bot])

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants