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.

Sign up for GitHub

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

Jump to bottom

Inconsistent lockfile changes on repeat installs #7444

Closed
1 of 4 tasks
alex-statsig opened this issue Dec 19, 2023 · 14 comments · Fixed by #7583 or #7606
Closed
1 of 4 tasks

Inconsistent lockfile changes on repeat installs #7444

alex-statsig opened this issue Dec 19, 2023 · 14 comments · Fixed by #7583 or #7606
Assignees
@zkochan
Labels
type: bug

Comments

@alex-statsig
Copy link

Verify latest release

  • I verified that the issue exists in the latest pnpm release

pnpm version

7.33.5 (latest-7), as well as 7.32.4 and 7.28.0

Which area(s) of pnpm are affected? (leave empty if unsure)

Lockfile

Link to the code that reproduces this issue or a replay of the bug

No response

Reproduction steps

Hard to create a consistent repro, it occurs with a large project. One example dependency which triggers it is jest-config@29.6.1(@types/node@16.11.58)(ts-node@10.9.1). It seems the peer version of @types/node changes across installs.

Describe the Bug

Repeat installs cause several inconsistent changes to the lockfile:

  1. "dev: true" flag disappears and reappears
  2. Version of peer @types/node (using new lockfile format) within ts-node changes:

Ex. on one install I get the following lockfile section:

  /jest-config@29.6.1(@types/node@16.11.58)(ts-node@10.9.1):
    resolution: {integrity: sha512-XdjYV2fy2xYixUiV2Wc54t3Z4oxYPAELUzWnV6+mcbq0rh742X2p52pii5A3oeRzYjLnQxCsZmp0qpI6klE2cQ==}
    engines: {node: ^14.15.0 || ^16.10.0 || >=18.0.0}
    peerDependencies:
      '@types/node': '*'
      ts-node: '>=9.0.0'
    peerDependenciesMeta:
      '@types/node':
        optional: true
      ts-node:
        optional: true
    dependencies:
      '@babel/core': 7.19.0
      '@jest/test-sequencer': 29.6.1
      '@jest/types': 29.6.1
      '@types/node': 16.11.58
      babel-jest: 29.6.1(@babel/core@7.19.0)
...
      ts-node: 10.9.1(@types/node@16.11.22)(typescript@4.8.4)
    transitivePeerDependencies:
      - supports-color

On another install, I get the following:

  /jest-config@29.6.1(@types/node@16.11.58)(ts-node@10.9.1):
    resolution: {integrity: sha512-XdjYV2fy2xYixUiV2Wc54t3Z4oxYPAELUzWnV6+mcbq0rh742X2p52pii5A3oeRzYjLnQxCsZmp0qpI6klE2cQ==}
    engines: {node: ^14.15.0 || ^16.10.0 || >=18.0.0}
    peerDependencies:
      '@types/node': '*'
      ts-node: '>=9.0.0'
    peerDependenciesMeta:
      '@types/node':
        optional: true
      ts-node:
        optional: true
    dependencies:
      '@babel/core': 7.19.0
      '@jest/test-sequencer': 29.6.1
      '@jest/types': 29.6.1
      '@types/node': 16.11.58
      babel-jest: 29.6.1(@babel/core@7.19.0)
...
      ts-node: 10.9.1(@types/node@16.9.2)(typescript@4.8.4)
    transitivePeerDependencies:
      - supports-color

The only difference is ts-node: 10.9.1(@types/node@16.11.22)(typescript@4.8.4) changing to ts-node: 10.9.1(@types/node@16.9.2)(typescript@4.8.4)

Expected Behavior

Lockfile should not change for a consistent version of pnpm. This causes lots of pain with merge conflicts as the lockfile thrashes back and forth in many PRs.

Which Node.js version are you using?

v16.17.1

Which operating systems have you used?

  • macOS
  • Windows
  • Linux

If your OS is a Linux based, which one it is? (Include the version if relevant)

No response
@zkochan
Copy link
Member

zkochan commented Dec 24, 2023

@stevenpetryk are these the same issues that you were looking into?

@stevenpetryk
Copy link
Contributor

@zkochan we have observed the exact same two things (dev: true disappearing/reappearing as well as @types/node resolution changing in particular), but I'm not sure if it's the same issue as #7393 because we have not been able to reproduce the issue using the same strategy (or really reproduce it at all outside of the discord monorepo).

My hypothesis with all three behaviors is that they're some sort of race condition during resolution, but I haven't spent any time debugging yet.

@zkochan
Copy link
Member

zkochan commented Dec 24, 2023

I can reproduce one issue with @types/node on the pnpm repository. If I update @rushstack/worker-pool, @types/node is removed from the package key in the lockfile

image

@zkochan
Copy link
Member

zkochan commented Dec 24, 2023

@alex-statsig in any case, you should upgrade to pnpm v8. We only ship security patches to pnpm v7.

@alex-statsig
Copy link
Author

@alex-statsig in any case, you should upgrade to pnpm v8. We only ship security patches to pnpm v7.

Thanks, we are working on it but just having some trouble keeping versions pinned during the upgrade (some typescript errors introduced, mostly from mui, which doesn't seem correct)

@KSXGitHub
Copy link
Contributor

@alex-statsig Can you give me access to your large project? I only need the package.json, the lockfile, and other the configuration files that are relevant to pnpm.

@alex-statsig
Copy link
Author

Sure, the repo as a whole is private; what's a good way to send you those files?

@KSXGitHub
Copy link
Contributor

KSXGitHub commented Jan 9, 2024 •
edited
Loading

I only need the pnpm related files (like package.json, .npmrc, pnpm-workspace.yaml, etc). Are you allowed to remove all your code except those files then publish them to a public repo?

@jgao54
Copy link

jgao54 commented Jan 20, 2024

We are also observing this with v7.33.6 in a large monorepo.

Seeing inconsistency in @types/node and ts-node, but also other dependencies as well.

@zkochan zkochan self-assigned this Jan 28, 2024
@zkochan zkochan added this to Status Jan 28, 2024
@zkochan zkochan moved this to in progress in Status Jan 28, 2024
@zkochan
Copy link
Member

zkochan commented Jan 28, 2024

Oh, this is a complex scenario. ts-node is an optional peer dependency and it depends on another optional peer dependency. As a workaround, you can try to set dedupe-peer-dependents to false.

zkochan added a commit that referenced this issue Jan 28, 2024
@zkochan
fix: resolve peer having peer correctly
737872e 
close #7444
@zkochan zkochan mentioned this issue Jan 28, 2024
Merged
@zkochan
Copy link
Member

zkochan commented Jan 29, 2024

After all, I don't think changing dedupe-peer-dependents will help. I am working on a fix.

zkochan added a commit that referenced this issue Jan 30, 2024
@zkochan
fix: resolve peer having peer correctly
ad83473 
close #7444
zkochan added a commit that referenced this issue Jan 30, 2024
@zkochan
fix: resolve peer having peer correctly
a76816e 
close #7444
zkochan added a commit that referenced this issue Jan 30, 2024
@zkochan
fix: resolve peer having peer correctly (#7583)
977060f 
close #7444
zkochan added a commit that referenced this issue Jan 30, 2024
@zkochan
fix: resolve peer having peer correctly (#7583)
44062ee 
close #7444
@zkochan
Copy link
Member

zkochan commented Feb 2, 2024

Unfortunately this one will be hard to fix. The peers resolution algorithm will have to go through some heavy refactoring. And this will be a breaking change. But it is important to fix it. I will try to do it in v9.

This happens when peer dependencies have peer dependencies of its own. I thought I have fixed it in #7583 but no, the problem requires a much bigger rewrite.

@zkochan zkochan reopened this Feb 2, 2024
@zkochan zkochan mentioned this issue Feb 6, 2024
Merged
@zkochan
Copy link
Member

zkochan commented Feb 8, 2024

The fix is ready for review: #7606

zkochan added a commit that referenced this issue Feb 8, 2024
@zkochan
fix: resolve peer of peer from the deps of direct dependent package (#…
98a1266 
…7606)

close #7444

Peer dependencies of peer dependencies are now resolved correctly. When peer dependencies have peer dependencies of their own, the peer dependencies are grouped with their own peer dependencies before being linked to their dependents.

For instance, if `card` has `react` in peer dependencies and `react` has `typescript` in its peer dependencies, then the same version of `react` may be linked from different places if there are multiple versions of `typescript`. For instance:

```
project1/package.json
{
  "dependencies": {
    "card": "1.0.0",
    "react": "16.8.0",
    "typescript": "7.0.0"
  }
}
project2/package.json
{
  "dependencies": {
    "card": "1.0.0",
    "react": "16.8.0",
    "typescript": "8.0.0"
  }
}
node_modules
  .pnpm
    card@1.0.0(react@16.8.0(typescript@7.0.0))
      node_modules
        card
        react --> ../../react@16.8.0(typescript@7.0.0)/node_modules/react
    react@16.8.0(typescript@7.0.0)
      node_modules
        react
        typescript --> ../../typescript@7.0.0/node_modules/typescript
    typescript@7.0.0
      node_modules
        typescript
    card@1.0.0(react@16.8.0(typescript@8.0.0))
      node_modules
        card
        react --> ../../react@16.8.0(typescript@8.0.0)/node_modules/react
    react@16.8.0(typescript@8.0.0)
      node_modules
        react
        typescript --> ../../typescript@8.0.0/node_modules/typescript
    typescript@8.0.0
      node_modules
        typescript
```

In the above example, both projects have `card` in dependencies but the projects use different versions of `typescript`. Hence, even though the same version of `card` is used, `card` in `project1` will reference `react` from a directory where it is placed with `typescript@7.0.0` (because it resolves `typescript` from the dependencies of `project1`), while `card` in `project2` will reference `react` with `typescript@8.0.0`.
@zkochan
Copy link
Member

zkochan commented Feb 9, 2024 •
edited
Loading

🚢 9.0.0-alpha.4.

@anomiex anomiex mentioned this issue Oct 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zkochan zkochan

Labels
type: bug
Projects
Status
Archived in project
Milestone
No milestone
5 participants
@stevenpetryk @zkochan @KSXGitHub @jgao54 @alex-statsig