yarnpkg / berry

πŸ“¦πŸˆ Active development trunk for Yarn βš’
https://yarnpkg.com
BSD 2-Clause "Simplified" License
7.43k stars 1.11k forks source link

Surprises around `resolutions` #2202

Open borekb opened 3 years ago

borekb commented 3 years ago

I encountered a few "surprises" around resolutions – not sure if those are expected but since I couldn't find them mentioned on GitHub yet, here they go.

Context

We're using graphql@14 in our project but one of the deep dependencies has a peerDependency on graphql@15, which results in a warning like this:

➀ YN0060: β”‚ @company/my-package@workspace:my-package provides graphql (pc69a0) with version 14.7.0, which doesn't satisfy what @graphql-codegen/typescript-operations and some of its descendants request

The actual package that has this peerDependency is relay-compiler@10.0.1. Since we cannot upgrade graphql in our project, the "solution" (though not great) is to downgrade relay-compiler to 9.x which has peerDependency on graphql@14.

--save / -s doesn't work

I ran this command:

yarn set resolution --save relay-compiler@npm:10.0.1 9.1.0

This updated yarn.lock like this (will be important later):

"@graphql-tools/relay-operation-optimizer@npm:^6":
  version: 6.2.4
  resolution: "@graphql-tools/relay-operation-optimizer@npm:6.2.4"
  dependencies:
    relay-compiler: 10.0.1

"relay-compiler@npm:10.0.1":
  version: 9.1.0
  resolution: "relay-compiler@npm:9.1.0"
  dependencies: ...

But it didn't affect project-level package.json – I expected to find resolutions field there. According to this Discord reply, -s isn't implemented yet. Fair enough but I couldn't find the info here on GitHub so just want to mention it.

resolutions field in package.json doesn't update yarn.lock?

Since I wanted to have a mention of a version mapping in package.json and not just in yarn.lock, I updated the manifest manually:

{
  "name": "@company/project-root",
  "resolutions": {
    "relay-compiler": "9.1.0"
  }
}

I then reverted yarn.lock (I got rid of the changes done by yarn set resolution) and ran yarn (install).

At this point, I expected the yarn.lock to look exactly like above but this was the result instead:

"@graphql-tools/relay-operation-optimizer@npm:^6":
  version: 6.2.4
  resolution: "@graphql-tools/relay-operation-optimizer@npm:6.2.4"
  dependencies:
    relay-compiler: 10.0.1

"relay-compiler@npm:9.1.0":
  version: 9.1.0
  resolution: "relay-compiler@npm:9.1.0"
  dependencies: ...

The key point is that @graphql-tools/relay-operation-optimizer depends on relay-compiler@10.0.1 but this version 10.0.1 is nowhere to be found in the lockfile – the only key there is "relay-compiler@npm:9.1.0".

I guess in this case, Yarn consults both yarn.lock and package.json#resolutions to find out how to resolve relay-compiler but it feels strange to me – I'd expect the mapping to be obvious just from the lockfile itself.


Again, not sure if those are issues or expected behavior but they surprised me.

arcanis commented 3 years ago

resolutions field in package.json doesn't update yarn.lock?

This is expected - the resolutions field only has effect while it's there. It's important, otherwise your ranges would be "eternally" locked to likely incompatible range resolutions.

--save / -s doesn't work

Indeed, we should remove this flag from the documentation for the time being (or implement it) πŸ€”

borekb commented 3 years ago

@arcanis thanks for the explanation. I get what you're saying but it still feels strange that resolutions vs. yarn set resolution lead to two different end states of our source files (although the actual installation will be the same, at least I hope πŸ˜„).

Theoretically, could yarn set resolution update only the package.json manifest instead of writing to yarn.lock? That way, both ways would be equivalent.

arcanis commented 3 years ago

Theoretically, could yarn set resolution update only the package.json manifest instead of writing to yarn.lock? That way, both ways would be equivalent.

No; the resolutions field and yarn set resolution are two different things. The first is a core-supported feature in itself, the second is meant to do exactly what it does: assign a new resolution to the lockfile. They share the same name because resolution is unfortunately a bit too generic, but they have different purposes.

arcanis commented 3 years ago

Reopening for --save

borekb commented 3 years ago

Oh, so the two are two different things? Even though the --save is currently not implemented, its docs say this:

If you wish to make the enforced resolution persist whatever happens, add the -s,--save flag which will also edit the resolutions field from your top-level manifest.

This makes intuitive sense to me but it's possible that this docs suggest something that couldn't work / is wrong.

yarnbot commented 3 years ago

Hi! πŸ‘‹

This issue looks stale, and doesn't feature the reproducible label - which implies that you didn't provide a working reproduction using Sherlock. As a result, it'll be closed in a few days unless a maintainer explicitly vouches for it or you edit your first post to include a formal reproduction (you can use the playground for that).

Note that we require Sherlock reproductions for long-lived issues (rather than standalone git repositories or similar) because we're a small team. Sherlock gives us the ability to check which bugs are still affecting the master branch at any given point, and decreases the amount of code we need to run on our own machines (thus leading to faster bug resolutions). It helps us help you! πŸ˜ƒ

If you absolutely cannot reproduce a bug on Sherlock (for example because it's a Windows-only issue), a maintainer will have to manually add the upholded label. Thanks for helping us triaging our repository! 🌟

noahnu commented 3 years ago

What does a "--save" look like? I was playing around with adding save functionality, and the main issue is that if you're setting a resolution for a transitive dependency, you need to specify the full path as the resolution key in the package.json. We can't fallback to globs because that's been removed in yarn v2 (unless it were to be added back?). Is the idea to only support saving for direct dependencies, or do we need to somehow generate the patterns?

I took a stab and trying to generate the patterns, however I don't think there's an existing function which given a descriptor, lists all packages that reference that descriptor? Also ran into an issue comparing descriptors because the npm: protocol is optional, but is not inferred when generating descriptor hashes (ended up re-generating the hashes).

cianx commented 3 years ago

It appears the -s and --save are still not implemented in 3.0.1. Resolutions only specified in the lock file are not very user friendly as they cannot be easily observed by a developer. The lock file is expected to be a result of configuration not a means of overriding it.

KyloJorgensen commented 2 years ago

It might be helpful if this is resolved aka faker.js and color.js

MrChrisRodriguez commented 2 years ago

The documentation is still incorrect/confusing. The -s / --save flag is still being shown even though it doesn't do anything which leads devs down a rabbit whole looking for a bug instead of just knowing there's an absent feature.

0x80 commented 2 years ago

I'm trying to use resolutions inside of a yarn workspaces repo, because it seems I have a problem with react 18 types clashing with 17 when attempting to use React 17 in a Nextjs 12 app, so this is what I've tried:

yarn set resolution @types/react@npm:18.0.9 17.0.49

I've tried this in the root of the repo and inside the app workspace package, but I see no changes made to the lock file or any other file. Am I missing something?

steverep commented 1 year ago

Found this issue after being very confused why -s seemed to do nothing...

No; the resolutions field and yarn set resolution are two different things. The first is a core-supported feature in itself, the second is meant to do exactly what it does: assign a new resolution to the lockfile. They share the same name because resolution is unfortunately a bit too generic, but they have different purposes.

I'm also confused by this explanation. If they have different purposes, could you give an example of why you'd prefer to use one method over the other?

If yarn set resolution is intended to produce a different result (and not add to the "resolutions" field), then that just leads to more questions like:

  1. How do you know it was set by a developer? Nothing in the lock file change suggests as much.
  2. How do you undo it other than edit the lock file manually?
  3. Why is yarn patch so different (because its --save works as expected)?
Blasz commented 1 year ago

resolutions dependencies not updating yarn.lock is making it harder for me to parse yarn.lock as I now need to also cross-reference the resolutions field to find missing ranges in yarn.lock.

This is expected - the resolutions field only has effect while it's there. It's important, otherwise your ranges would be "eternally" locked to likely incompatible range resolutions.

Couldn't this be solved by not allowing incompatible range resolutions unless explicitly specified in resolutions, similar to what yarn 1 does? I'm assuming allowing incompatible ranges anywhere without resolutions is an intentional decision, what is the reasoning behind it?

Alternatively could something like a resolution: protocol be used to signal that the range is there because of a resolution? When processing the lockfile yarn could then cross-reference that with the resolutions field to see if it is still valid. I'm guessing this wouldn't work since the protocol couldn't actually be used in the resolution field of yarn.lock

gertsonderby commented 1 year ago

As far as I can tell, the only thing that has changed since this issue was first posted in 2020, is that resolutions in package.json just plain do nothing now, instead of maybe working a bit weirdly like the OP describes. (--save still remains in the documentation, also.)

My own results using yarn@3.2.0:

Excerpt from my package.json:

  "resolutions": {
    "@types/react": "17.0.43",
    "uglify-js@npm:~1.2": "2.6.0",
    "underscore@npm:~1.4.4": "1.12.1"
  },

Output of yarn why uglify-js, note the middle result:

β”œβ”€ handlebars@npm:4.7.7
β”‚  └─ uglify-js@npm:3.17.4 (via npm:^3.1.4)
β”‚
β”œβ”€ juicer@npm:0.6.15
β”‚  └─ uglify-js@npm:1.2.6 (via npm:~1.2)
β”‚
└─ pug-filters@npm:3.1.1
   └─ uglify-js@npm:2.8.29 (via npm:^2.6.1)

Excerpt from my yarn.lock as created from scratch from the above package.json:


"uglify-js@npm:^2.6.1":
  version: 2.8.29
  resolution: "uglify-js@npm:2.8.29"
  dependencies:
    source-map: ~0.5.1
    uglify-to-browserify: ~1.0.0
    yargs: ~3.10.0
  dependenciesMeta:
    uglify-to-browserify:
      optional: true
  bin:
    uglifyjs: bin/uglifyjs
  checksum: 24f2ae09b96bbb56cc3802f575ee2cdbc6822d942c6877ee4a5637e949f269e48f4baa8d193c47324cdfc1cc8e6853e1479d26e228be2412bc0da3649eaedb35
  languageName: node
  linkType: hard

"uglify-js@npm:^3.1.4":
  version: 3.17.4
  resolution: "uglify-js@npm:3.17.4"
  bin:
    uglifyjs: bin/uglifyjs
  checksum: 7b3897df38b6fc7d7d9f4dcd658599d81aa2b1fb0d074829dd4e5290f7318dbca1f4af2f45acb833b95b1fe0ed4698662ab61b87e94328eb4c0a0d3435baf924
  languageName: node
  linkType: hard

"uglify-js@npm:~1.2":
  version: 1.2.6
  resolution: "uglify-js@npm:1.2.6"
  bin:
    uglifyjs: ./bin/uglifyjs
  checksum: dbabf3ad42a226c1b70e799331f084f8b2ce7b013266565a4ea0b7cf461c09912dbbae07ac292480db4debc2777e14ab2c4e50eed8bba5e94efd1195052ef38d
  languageName: node
  linkType: hard

As you can see, yarn just went and installed the old version of uglify-js despite any protestations of package.json. No sign of the package it was supposed to redirect to (v2.6.0). Does juicer use a newer version of it? I don't know -- does anyone know a way to check that? Dependabot certainly seems to think not.

sushruth commented 1 year ago

@gertsonderby I have had this issue before with yarn. My workaround was to not use semver ranges but use exact versions as reported by yarn why and it would fix that. But that's not a good solution. We need yarn to treat semver ranges in resolutions properly.

jaybuidl commented 1 year ago

But why not mentioning that -s is not implemented on the docs? https://yarnpkg.com/cli/set/resolution

PaulRBerg commented 1 year ago

Ok so how can resolutions be enforced in Yarn v3? It's not clear from this conversation.

Or can they not be enforced at all?

gertsonderby commented 1 year ago

Ok so how can resolutions be enforced in Yarn v3? It's not clear from this conversation.

You cannot get yarn to follow resolutions set in package.json to my knowledge -- it simply ignores them.

You need to set each resolution with yarn itself using the yarn set resolution command, which requires either a package name with no version at all (which resolves the entire package in all versions), or the exact included version as found by yarn why <package>. So for example, the following:

> yarn why uglify-js
[... some output from yarn ...]
β”œβ”€ juicer@npm:0.6.15
β”‚  └─ uglify-js@npm:1.2.3 (via npm:~1.2)
[...]
> yarn set resolution uglify-js@npm:~1.2 2.6.0
[... a bunch of yarn output ...]
> yarn why uglify-js
[... some output from yarn ...]
β”œβ”€ juicer@npm:0.6.15
β”‚  └─ uglify-js@@npm:2.6.0 (via npm:~1.2)
[...]
>

(> is your command line, with commands you enter, [...] is output we're not interested in that I cut out for clarity.)

As you can probably see, you need the exact version that yarn is trying to resolve (the one from the 'via' part), and then it'll do the thing you want it to.

arcanis commented 1 year ago

You cannot get yarn to follow resolutions set in package.json to my knowledge -- it simply ignores them.

That's incorrect. I mean, we even use them on this very repository.

Since this issue is intended to be specifically about the -s flag (which isn't implemented right now, so you must set the resolutions field yourself as per the documentation syntax), I'll lock it until it's completed. If you find yourself in a situation where resolutions, when manually set, don't work, you can open a new issue with a reproduction - but I advise you to double-check your setup against this repo to make sure you're using the correct syntax.