yarnpkg / berry

📦🐈 Active development trunk for Yarn ⚒
https://yarnpkg.com
BSD 2-Clause "Simplified" License
7.41k stars 1.11k forks source link

[Undefined behavior]: `peerDependencies` prevents correct `node_modules` linking in workspaces #6558

Open GauBen opened 3 days ago

GauBen commented 3 days ago

Self-service

Describe the bug

With this monorepo structure

/
+-- package.json        # "name": "hello"
+-- index.js
\-- demo/
    +-- package.json    # "name:" "demo"
    \-- index.json

If demo/package.json declares a dependency on hello and nodeLinker: node-modules is set, the directory should look like this:

/
+-- package.json
+-- index.js
\-- demo/
    +-- node_modules/
    |   \-- hello        --> /
    +-- package.json
    \-- index.json

This is true unless hello declares a peer dependency!

If it declares a peer dependency, e.g.

{
  "peerDependencies": {
    "whatever": "*"
  },
  "peerDependenciesMeta": {
    "whatever": {
      "optional": true
    }
  }
}

Then /demo/node_modules/hello is no longer created

To reproduce

git clone https://github.com/GauBen/yarn-issue
cd yarn-issue
yarn
cd demo && yarn start
# Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'hello'

Removing peerDependencies and peerDependenciesMeta from the root package.json will fix the issue

Environment

System: OS: Linux 5.15 Ubuntu 22.04.5 LTS 22.04.5 LTS (Jammy Jellyfish) CPU: (12) x64 Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz Binaries: Node: 22.9.0 - /tmp/xfs-73d42128/node Yarn: 4.5.0 - /tmp/xfs-73d42128/yarn npm: 10.8.3 - ~/.pkgx/npmjs.com/v10.8.3/bin/npm

Additional context

Reproduction logs: https://github.com/GauBen/yarn-issue/actions/runs/11400676401/job/31722024945#step:5:22

GauBen commented 3 days ago

I don't know how to fix this, but with a few hints I should be able to write a patch and a test

larixer commented 3 days ago

If demo/package.json declares a dependency on hello and nodeLinker: node-modules is set, the directory should look like this:

It would create circular symlink, hence the behavior you expect is problematic.

GauBen commented 3 days ago

It works fine if peerDependencies gets removed: https://github.com/GauBen/yarn-issue/actions/runs/11401008393/job/31723033705?pr=1#step:5:18

There is indeed a circular symlink, but it does not seem to be an issue

larixer commented 3 days ago

There is indeed a circular symlink, but it does not seem to be an issue

The fact it is not an issue for you, doesn't mean it is not an issue for someone else. You are in a grey zone when you depend on the root workspace with node-moidules linker, the behavior is not guaranteed and there is no guarantee it will not break at some moment. This is a circular dependency between workspaces, we do support circular dependencies for normal dependencies, but for workspaces the behavior is not guaranteed.

Example of issues with circular symlinks (that are unavoidable in case you depend on root workspace) are in commens to the issue below: https://github.com/yarnpkg/berry/issues/3256

GauBen commented 3 days ago

Ok I get it, but is it a bug that declaring or removing peerDependencies changes the resulting node_modules?

the behavior is not guaranteed and there is no guarantee it will not break at some moment

Yarn should emit a warning in this case instead of failing silently as it is the case in the reproduction, if that's the road you're willing to take

larixer commented 3 days ago

Ok I get it, but is it a bug that declaring or removing peerDependencies changes the resulting node_modules?

Well, it can be viewed as a bug, and we can try to solve it. Still the layout of project you use is problematic and you might run in issues with this layout and then change it to non-circular one. And our efforts to fix a "bug" will be useless, that's my point. We can still fix this "bug" for the sake of some correctness in undefined situations, but it is a rabbit hole

GauBen commented 3 days ago

Ok I'd love to make the fact that this behavior is undefined clearer for future users, is a CLI warning the right way to do it?

I'd start with a simple foreach packages, if is_workspace_dependency && is_workspace_root then warn, would that make the cut?

Edit: just tested, there is no such issue with nodeLinker: pnp, the heuristic should only trigger in the node-modules linker, not during resolution

GauBen commented 3 days ago

I'm starting to investigate the rabbit hole: it also works with nodeLinker: pnpm. I guess this will be my work around for the time being. Thanks for your quick responses!

GauBen commented 3 days ago

Unrelated issue: declaring a nodeLinker that does not exist does not trigger an error, nodeLinker: ghjklm will fail silently

larixer commented 3 days ago

Yeah, non-existing linker worth fixing, might be not a trivial fix though, due to how plugins work