Yarn 4: Respect `enableTransparentWorkspaces: false` when resolving workspace dependencies

me4502 commented 3 months ago

Verify canary release

[X] I verified that the issue exists in the latest Turborepo canary release.

Link to code that reproduces this issue

https://github.com/me4502/turborepo-reproduction

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

Yarn v2/v3/v4 (node_modules linker only)

What operating system are you using?

Mac

Which canary version will you have in your reproduction?

2.0.12-canary.3

Describe the Bug

When a dependency using Yarn's npm: protocol exists that has the same name as a workspace in the repo, turborepo incorrectly builds the repo with that name. This causes issues in cases where FFI-requiring packages are built on CI and used purely via NPM rather than local workspaces.

Expected Behavior

As these dependencies come from npm, and not the workspace of the same name, the workspace should not be built nor considered part of turborepo's dependency graph.

To Reproduce

Clone https://github.com/me4502/turborepo-reproduction
Run yarn in the root of the repo
cd to workspaces/a
Run yarn build:turbo

As can be seen in the output, the buffer workspace's build script has been incorrectly run, despite it not being a dependency of the a workspace.

Additional context

The important parts of this setup are that there's a package called buffer being used by workspace a, and a workspace also named buffer. The repro is setup to specifically use the dependency from npm, so it should not be building the workspace copy as it's redundant (and in the case of FFI dependencies, potentially impossible from local development machines).

chris-olszewski commented 3 months ago

Thanks for the report. Just documenting for myself that we're doing the correct behavior for default Yarn 4 settings, but if enableTransparentWorkspaces: false is set, we need to change our dependency resolution behavior to respect that.

romanofski commented 1 month ago

I had a look at this out of curiosity. If I'm not mistaken the fix is likely addressing the TODO in crates/turborepo-repository/src/package_graph/dep_splitter.rs?:

    fn is_external(&self) -> bool {
        // The npm protocol for yarn by default still uses the workspace package if the
        // workspace version is in a compatible semver range. See https://github.com/yarnpkg/berry/discussions/4015
        // For now, we will just assume if the npm protocol is being used and the
        // version matches its an internal dependency which matches the existing
        // behavior before this additional logic was added.

        // TODO: extend this to support the `enableTransparentWorkspaces` yarn option
        self.protocol.map_or(false, |p| p != "npm")
    }

romanofski commented 3 weeks ago

@chris-olszewski I've hacked on this in a branch here https://github.com/romanofski/turborepo/commit/8b7166deb09842439c67b26b1fc47706ce9c70e1 If I did find the right locations to change, I wonder whether the config file representation for the package managers is not already a bottle neck.

Currently the npmrc and yarnrc configs are exposed as simple booleans, but perhaps should actually be represented more abstractly? Like instead of passing in the *rc instances in the DependencySplitter it might make sense to have an abstracted object exposing options for the dependency resolution. That can then be created with either the nprmc or yarnrc or whatever it is?

Anyway, guess if there is 5 mins to have a quick look at the branch/patch I'd appreciate some pointers in the right direction. I'll continue having a look at the integration tests since that's needed too.

vercel / turborepo