[Bug] Resolution algorithm in workspaces is unexpected since v2.4.x

gund commented 3 years ago

[ ] I'd be willing to implement a fix

Describe the bug

Before Yarn v2.4.0 all packages in the workspaces were resolved during the first install by finding the best versions to satisfy the most workspace packages.

However after updating to any version that is >= v2.4.0 resolution is no longer satisfying all workspace packages. And as a side effect this leads to duplicated packages being installed as described in #2665.

To Reproduce

Reproduction

```js repro // Sherlock reproduction const fs = require('fs') // Enable `node-modules` nodeLinker fs.writeFileSync('.yarnrc.yml', 'nodeLinker: node-modules'); // Setup Workspaces await packageJson({ name: 'root', workspaces: ['packages/*'], dependencies: { "@webcomponents/webcomponentsjs": "~2.4.0", "rxjs": "~6.6.0", "zone.js": "~0.10.3" }, devDependencies: { "@angular/core": "~9.1.12", "tslib": "~1.11.1", "typescript": "~3.8.3" }, resolutions: { "typescript": "3.8.3" } }) await packageJson({ name: 'dashboard', dependencies: { "@angular/core": "^9.1.12", "@spryker/button": "^0.2.0-next.9", "rxjs": "^6.5.4", "tslib": "^1.10.0" } }, { cwd: 'packages/dashboard' }) await packageJson({ name: 'security', dependencies: { "@angular/core": "^9.1.12", "@spryker/card": "^0.2.0-next.2", "rxjs": "^6.5.4", "tslib": "^1.10.0" } }, { cwd: 'packages/security' }) await packageJson({ name: 'zed-ui', dependencies: { "@angular/core": "^9.1.12", "@spryker/button": "^0.2.0-next.9", "@spryker/web-components": "^0.2.1-next.5", "@webcomponents/webcomponentsjs": "^2.4.1", "rxjs": "^6.5.4", "tslib": "^1.10.0" } }, { cwd: 'packages/zed-ui' }) // Perform install await yarn('install') // Check that all packages were hoisted in root expect(fs.existsSync('node_modules/@angular/core')).toBe(true) expect(fs.existsSync('node_modules/@spryker/button')).toBe(true) expect(fs.existsSync('node_modules/@spryker/card')).toBe(true) expect(fs.existsSync('node_modules/@spryker/web-components')).toBe(true) expect(fs.existsSync('node_modules/@webcomponents/webcomponentsjs')).toBe(true) expect(fs.existsSync('node_modules/rxjs')).toBe(true) expect(fs.existsSync('node_modules/tslib')).toBe(true) expect(fs.existsSync('node_modules/zone.js')).toBe(true) // Check that there are no packages in workspaces packages expect(fs.existsSync('packages/dashboard/node_modules')).toBe(false) expect(fs.existsSync('packages/product-list/node_modulesr')).toBe(false) expect(fs.existsSync('packages/product-offer/node_modules')).toBe(false) expect(fs.existsSync('packages/profile/node_modules')).toBe(false) expect(fs.existsSync('packages/sales-orders/node_modules')).toBe(false) expect(fs.existsSync('packages/security/node_modules')).toBe(false) expect(fs.existsSync('packages/zed-ui/node_modules')).toBe(false) ```

Environment if relevant (please complete the following information):

OS: WSL Ubuntu on Windows 10 x64
Node version v14.15.0
Yarn version >=v2.4.0

yarnbot commented 3 years ago

This issue reproduces on master:

Error: expect(received).toBe(expected) // Object.is equality

Expected: true
Received: false
    at module.exports (evalmachine.<anonymous>:60:55)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async /github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.2-91650a2501-627bee24a7.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:56:13
    at async executeInTempDirectory (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.2-91650a2501-627bee24a7.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:17:16)
    at async Object.executeRepro (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.2-91650a2501-627bee24a7.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:24:12)
    at async ExecCommand.execute (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.2-91650a2501-627bee24a7.zip/node_modules/@arcanis/sherlock/lib/commands/exec.js:25:38)
    at async ExecCommand.validateAndExecute (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-91cf93ba72.zip/node_modules/clipanion/lib/advanced/Command.js:161:26)
    at async Cli.run (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-91cf93ba72.zip/node_modules/clipanion/lib/advanced/Cli.js:74:24)
    at async Cli.runExit (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-91cf93ba72.zip/node_modules/clipanion/lib/advanced/Cli.js:83:28)

yarnbot commented 3 years ago

This issue reproduces on master:

Error: expect(received).toBe(expected) // Object.is equality

Expected: true
Received: false
    at module.exports (evalmachine.<anonymous>:60:55)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async /github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.2-91650a2501-627bee24a7.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:56:13
    at async executeInTempDirectory (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.2-91650a2501-627bee24a7.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:17:16)
    at async Object.executeRepro (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.2-91650a2501-627bee24a7.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:24:12)
    at async ExecCommand.execute (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-2.0.2-91650a2501-627bee24a7.zip/node_modules/@arcanis/sherlock/lib/commands/exec.js:25:38)
    at async ExecCommand.validateAndExecute (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-91cf93ba72.zip/node_modules/clipanion/lib/advanced/Command.js:161:26)
    at async Cli.run (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-91cf93ba72.zip/node_modules/clipanion/lib/advanced/Cli.js:74:24)
    at async Cli.runExit (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-91cf93ba72.zip/node_modules/clipanion/lib/advanced/Cli.js:83:28)

larixer commented 3 years ago

Because we talk about resolving ranges, the optimal resolution algorithm can be different for different people, The current resolution algorithm does not violate the package.json contract, that's why I don't see where is the bug. There are multiple metrics that can be optimized during resolution - the age of resolved packages, the total number of resolved packages, the severity of vulnerabilities in resolved packages, etc.

What might make sense is to allow plugins to override the default resolution algorithm. This is a difficult task though: how the API for the resolution plugin will look like? how different resolution plugins can interoperate with each other and with the default resolution logic? how the resolution plugins will interoperate with resolution protocol plugins? etc. Someone needs to come up with a proposal and dedicate a decent amount of time to development to make this really happen, they will also need to communicate and discuss all the aspects with the community.

arcanis commented 3 years ago

While what @larixer said is correct, it doesn't mean you are without options. The problem stems from one simple fact: Yarn will guarantee that a same range will always resolve to the same version. Because of this, if you want all your workspaces to use the exact same version of a package, you just need to specify the exact same dependency range (which isn't the case in your repro, cf @webcomponents/webcomponentsjs).

As for the general non-workspace case, deduplication is an optimization, and unoptimal deduplication shouldn't lead to functional changes. If it does, it's usually that something somewhere didn't use peer dependencies properly.

gund commented 3 years ago

@arcanis actually this bug shows that very same ranges will not always resolve to the same versions as I pointed out earlier it depends on current latest version of installed package in the registry.

I want to ask you one simple question and based on your answer we can decide whether this is a bug or not:

"If in workspace you have the package that is semantically compatible between all the workspace packages but it's versions are not exact same, how many versions of that package do you expect to have in your workspace?"

Exactly one: then this is a bug and has to be fixed
Many/it depends: then this is not a bug and you can go ahead and close the issue

I think as long as we do not agree on this fundamental question there is no point in discussing it further.

gund commented 3 years ago

I was just going through a PNPM workspaces feature and found out about their "shared-workspace-lockfile" which is exactly what I am looking for here:

all dependencies of workspace packages will be in a single [root] node_modules

So you can decide if you would like to enforce resolution to respect semver by default - or force it with the flag to install all packages as a singleton within a workspace.

gund commented 3 years ago

Hi @larixer @arcanis just a friendly ping. Would be great to have your thoughts regarding my 2 messages above 😃

Prior99 commented 3 years ago

I am seeing a similar issue. We have installed a dependency in multiple workspaces, all have the same version:

yarn why tsdi
├─ @myscope/a@workspace:packages/a [d40ac]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
├─ @myscope/a@workspace:packages/a [e8fa3]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
├─ @myscope/b@workspace:packages/b [3ea4a]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
├─ @myscope/b@workspace:packages/b [4ec04]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
├─ @myscope/b@workspace:packages/b [65985]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
├─ @myscope/b@workspace:packages/b [aed36]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
├─ @myscope/c@workspace:packages/c [e8fa3]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
├─ @myscope/d@workspace:packages/d [3ea4a]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
├─ @myscope/d@workspace:packages/d [4ec04]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
├─ @myscope/d@workspace:packages/d [c2c82]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
├─ @myscope/d@workspace:packages/d [d40ac]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
├─ @myscope/d@workspace:packages/d [f769b]
│  └─ tsdi@npm:0.25.0 (via npm:^0.25.0)
│
└─ e@workspace:packages/e
   └─ tsdi@npm:0.25.0 (via npm:^0.25.0)

Yet, yarn doesn't hoist them:

find -name tsdi
./packages/a/node_modules/tsdi
./packages/c/node_modules/tsdi
./packages/f/node_modules/tsdi
./node_modules/tsdi

merceyz commented 3 years ago

@Prior99 That doesn't look like the same issue, please open a new issue with a reproduction

tobilen commented 3 years ago

Hey,

thought i'd share workaround for sharing dependencies across workspaces with yarn berry. I use it for react-query, because i have a root level helper that pre-fills the react-query cache with data for my unit tests. If i were to include react-query in each package including the root level, react-query would not get hoisted, and my helper would set a different context than the one used by the code that is being tested.

1. Create a new workspace package.

Relevant part from the package.json:

{
  "name": "@my-monorepo-name/react-query",
  "version": "0.1.0",
  "license": "UNLICENSED",
  "private": true,
  "files": [
    "/index.ts"
  ],
  "dependencies": {
    "react": "^17.0.1",
    "react-dom": "^17.0.1",
    "react-query": "^3.19.6"
  }
}

index.ts:

export * from "react-query";

2. Replace `react-query: "..."` in each package with: `"@my-monorepo-name/react-query": "workspace:*"`

The workspace:* ensures that we'll always resolve to the local version and never fetch from npm.

3. Replace imports in files

For example, replace import { useQuery } from "react-query" with import { useQuery } from "@my-monorepo-name/react-query"

4. Bonus: Restrict `react-query` imports with eslint

You can use the no-restricted-imports eslint plugin to prevent certain imports: https://eslint.org/docs/rules/no-restricted-imports .eslintrc.js:

module.exports = {
  // ...
  rules: {
    "no-restricted-imports": [
      "error",
      {
        paths: [
          {
            name: "react-query",
            message:
              "Import '@my-monorepo-name/react-query' to use the shared react-query instance.",
          },
        ],
      },
    ],
  }
}

Hope this helps with your issue 👋

tomskopek commented 1 year ago

@tobilen This is more than 1 year old, but I just wanted to say that your comment really helped me! Thanks 😄

gund commented 1 year ago

Unfortunately the issue @tobilen was talking about is not relevant as this issue is about broken version resolution in yarn workspaces in general and it's inability to properly resolve one common semver-compatible version between all packages in a workspace. All other workspaces implementations (NPM, PNPM and even Yarn v1) are working as expected in this regard btw.

As maintainers are simply ignoring it and unwilling to understand/fix it - we've moved on to NPM workspaces and it's doing the job as expected.

yarnpkg / berry