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

Yarn Berry Turbo Prune minor incorrectness #2049

Closed
quinnturner opened this issue Sep 22, 2022 · 10 comments
Closed

Yarn Berry Turbo Prune minor incorrectness #2049

quinnturner opened this issue Sep 22, 2022 · 10 comments
Assignees
Labels
kind: bug Something isn't working needs: author input

Comments

@quinnturner
Copy link

quinnturner commented Sep 22, 2022

What version of Turborepo are you using?

v1.5.1 v1.5.6

What package manager are you using / does the bug impact?

Yarn v2/v3 (node_modules linker only)

What operating system are you using?

Mac

Describe the Bug

First, congrats to @chris-olszewski on implementing this feature; it is a challenging engineering problem!

I tried to prune my pretty extensive monorepo, and it was so close to supporting --immutable. Here is the diff after running yarn:

-"@types/node@npm:*, @types/node@npm:>=12, @types/node@npm:>=12.12.47, @types/node@npm:>=13.7.0, @types/node@npm:^16.11.59":
+"@types/node@npm:*, @types/node@npm:>=12.12.47, @types/node@npm:>=13.7.0, @types/node@npm:^16.11.59":
  version: 16.11.59
  resolution: "@types/node@npm:16.11.59"
  checksum: b3a2986ef8f8216bb7fd1187dcf2e69aca3e6b6b5fdfd6abf4cf11c44e3cdd587fae875a28dc4ef257e2b369e2c5290642aff6a0a9d702f3385a78f561293259
  languageName: node
  linkType: hard

Where the usages of @types/node >= 12 are as follows:

"@commitlint/load@npm:>6.1.1":
  version: 17.0.0
  resolution: "@commitlint/load@npm:17.0.0"
  dependencies:
    "@commitlint/config-validator": ^17.0.0
    "@commitlint/execute-rule": ^17.0.0
    "@commitlint/resolve-extends": ^17.0.0
    "@commitlint/types": ^17.0.0
    "@types/node": ">=12"
    chalk: ^4.1.0
    cosmiconfig: ^7.0.0
    cosmiconfig-typescript-loader: ^2.0.0
    lodash: ^4.17.19
    resolve-from: ^5.0.0
    typescript: ^4.6.4
  checksum: c35f7c5d7a8e2812a62b2d10f304c4b4ab8754923b0e59c66f3d9ce5ff3ea84502539c905b82fafbe666b02db5e7d818c119764af5d46485532a8bb73dba0661
  languageName: node
  linkType: hard

"meros@npm:^1.1.4":
  version: 1.2.0
  resolution: "meros@npm:1.2.0"
  peerDependencies:
    "@types/node": ">=12"
  peerDependenciesMeta:
    "@types/node":
      optional: true
  checksum: 95ec2dc352ad4ffe7a8572676d0399e8cfe67bb3ebe03ce5b03ebb4d990527994528260ff47a7efedc1348a8ee46bfe782564127bd1566d4790e1189b0a1ed29
  languageName: node
  linkType: hard

It also got a patch slightly incorrect, but you noted that in the feature PR description. The patch does not seem to be related to the @types/node issue.

Expected Behavior

The prune to produce a lockfile that supports the --immutable flag.

To Reproduce

Unfortunately, I can't share the lockfile over here. If you're interested in a proper reproduction it will take some time to isolate the specific issue.

Best of luck!

@chris-olszewski chris-olszewski added the kind: bug Something isn't working label Sep 22, 2022
@chris-olszewski
Copy link
Member

Thanks for the report. Is there any chance you could get me the entries that reference the other versions of @types/node?

@chris-olszewski chris-olszewski self-assigned this Sep 22, 2022
@chris-olszewski
Copy link
Member

Are the entries for @commitlint/load@npm:>6.1.1" and meros@npm:^1.1.4 that you gave above from the pruned lockfile?

@quinnturner
Copy link
Author

Pre-prune, there are no references to >=12, meros/commitlint post-prune.

I will try and put together a better reproduction if possible tomorrow/this weekend.

Thanks!

@chris-olszewski
Copy link
Member

Hey @quinnturner, any chance you could get me all of the pre-prune lockfile entries that reference @types/node? I've been trying to repro this and haven't had any luck.

@quinnturner
Copy link
Author

@chris-olszewski I have created a documented reproduction and invited you to the private GitHub repository. Expect an email for approval! The reproduction is on v1.5.6.

@chris-olszewski
Copy link
Member

@quinnturner, thank you so much for the reproduction. I just put up #2269 which fixes 2/3 changes, the final one comes from the turbo not fully understanding how the resolutions section of package.json impacts yarn.lock. It's doable to fix this, but it'll be a more involved PR.

@nathanhammond
Copy link
Contributor

(Targeting 1.6.1 with this one.)

kodiakhq bot pushed a commit that referenced this issue Oct 26, 2022
Partially addresses #2049 

Yarn 2+ doesn't expect descriptors that appear in `peerDependencies` to be kept. This behavior only manifested in a lockfile where a pruned package had a normal dependency descriptor that also appeared as a peer dependency descriptor in a package that wasn't pruned. As background "descriptor" is the term Yarn uses for a semver range that appears in a `package.json` or lockfile entry.

Amended the minimal berry lockfile to now test that we don't keep around descriptors even if they appear in `peerDependencies`.
@PatrickLef
Copy link

@chris-olszewski Appreciate the good work! We're also running into the same issue as mentioned above, with both resolutions and peerDependencies. This is currently blocking our transition into Yarn 2, are there any easy workarounds in the meanwhile?

@quinnturner
Copy link
Author

@PatrickLef in my opinion, use --no-immutable until all Yarn Berry lockfile issues are resolved. There are so few inconsistencies now that the added risk of having the installation mutable likely doesn't increase security concern scope.

@quinnturner
Copy link
Author

I am closing this ticket now as the issues initially outlined have been resolved except for resolutions. For resolving the resolutions, there's #2791.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working needs: author input
Projects
None yet
Development

No branches or pull requests

4 participants