turbo icon indicating copy to clipboard operation
turbo copied to clipboard

fix: upgrade the turbo package in Yarn 4

Open atimmer opened this issue 1 year ago • 4 comments

Description

This fixes a failed install when running npx @turbo/codemod@latest update while using Yarn 4.

Screenshot 2024-05-03 at 10 26 15

yarn add turbo@latest --dev -W is not a valid command: Screenshot 2024-05-03 at 10 27 05

Using yarn version 4.1.0: Screenshot 2024-05-03 at 10 27 29

It seems to me that -W is not a valid flag for most yarn commands. The only command I could find is yarn workspaces foreach, but that's not the command being issued in this code.

I don't have a great understanding of this codebase, so I did my best to create a version that works. It looks to me that the code is trying to be resilient against which directory the user is at. So I replicated that behavior using yarn workspaces foreach --all --include .. It's a trick to run a specific command in the root of the workspace regardless of current location.

I wasn't able to build the repo. I was hoping for a TS only way to build only the codemod package, but I didn't find it. I currently don't have the time to setup the Go & Rust toolchain and I've never used either. I tested this by manually assembling the command from the code and testing that in my repo.

Testing Instructions

  1. Go to a repo with an outdated version of Turbo & Yarn 4 as the package manager.
  2. Run the codemod command
  3. It should now work after this change.

atimmer avatar May 03 '24 08:05 atimmer

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-basic-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2025 3:20am
examples-designsystem-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2025 3:20am
examples-gatsby-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2025 3:20am
examples-kitchensink-blog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2025 3:20am
examples-native-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2025 3:20am
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2025 3:20am
examples-svelte-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2025 3:20am
examples-tailwind-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2025 3:20am
examples-vite-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 18, 2025 3:20am

vercel[bot] avatar May 03 '24 08:05 vercel[bot]

@atimmer is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar May 03 '24 08:05 vercel[bot]

Hey, thanks for the contribution! I unfortunately tried this with Yarn 1 and it doesn't work there. So we'd be trading Yarn 1 for Yarn 4. 😄

Is it possible you could run yarn --version prior to that code block and then fork the behavior based on the version that comes back? I know that adds some difficulty to things but I'd like to be making an explicit move towards completeness than trading one for the other.

(Alternatively, if you'd like to leave it to me to pick up what you've written here and adapt it for those requirements, I can do that as well. I've had it happen to me where I tried to submit a PR and the maintainer asked for more and I thought "I had time for the simple way, not the hard way", so I'd completely understand. However, some folks like to see their name in the release notes so don't want to take that away from you!)

anthonyshew avatar Jun 15 '24 23:06 anthonyshew

@anthonyshew It seems like there was already a code block for detecting the Yarn version. And somehow I put the change in the wrong one, oops. Because I wasn't able to test this I didn't detect this. I have now rebased the PR to put the codeblock in the correct position. This assumes that there is no difference between Yarn 2, 3 and 4 in this regard.

(Alternatively, if you'd like to leave it to me to pick up what you've written here and adapt it for those requirements, I can do that as well. I've had it happen to me where I tried to submit a PR and the maintainer asked for more and I thought "I had time for the simple way, not the hard way", so I'd completely understand. However, some folks like to see their name in the release notes so don't want to take that away from you!)

In any case you can always take my code and make it shippable. My name in the release notes is not that important to me. My main concern is improving the project and removing a small annoyance from my workflow 😄

atimmer avatar Jul 08 '24 09:07 atimmer

Hi! 👋 I've run into this issue as well and was looking to fix it before seeing that this issue already existed.

I found that even though this code technically works:

case "yarn":
      // yarn 2.x and 3.x (berry)
      if (gte(packageManagerVersion, "2.0.0")) {
        return renderCommand([
          "yarn",
          "add",
          `turbo@${to}`,
          installType === "devDependencies" && "--dev",
        ]);
        // yarn 1.x
      }
      return renderCommand([
        "yarn",
        "add",
        `turbo@${to}`,
        installType === "devDependencies" && "--dev",
        isUsingWorkspaces && "-W",
      ]);

The packageManagerVersion gotten in the getAvailablePackageManagers method does not seem to be correct since it's running on a tmp directory instead of the repository root. According to Yarn's documentation, its binary preferably should be on each repository. So this way, when I run @turbo/codemod update on my repository which has a local Yarn version, it detects version 1 (that is installed by default globally) when it should be the local version 4.

I found that if I disable running on tmp folder for each command, the returned version is actually correct:

async function exec(command: string, args: Array<string> = [], opts?: Options) {
  // run the check from tmpdir to avoid corepack conflicting -
  // this is no longer needed as of https://github.com/nodejs/corepack/pull/167
  // but we'll keep the behavior for those on older versions)
  const execOptions: Options = {
    // REMOVE THIS LINE HERE
    // cwd: os.tmpdir(),
    env: { COREPACK_ENABLE_STRICT: "0" },
    ...opts,
  };
  try {
    const { stdout } = await execa(command, args, execOptions);
    return stdout.trim();
  } catch {
    return undefined;
  }
}

But I'm unaware if this breaks other package managers. @anthonyshew do you think this is a viable fix?

tomcastro avatar Nov 21 '24 14:11 tomcastro

@tomcastro It may be. Could you open up a separate PR with that so I can take a look at it?

anthonyshew avatar Nov 21 '24 20:11 anthonyshew

@anthonyshew sure thing, created this PR

tomcastro avatar Nov 28 '24 12:11 tomcastro

@atimmer, hope you don't mind but I'm going to try something on this PR.

It looks like you're both on the right track:

  • We're finding the wrong binary for yarn berry versions. The tmpdir meant we were exiting the repository's context, so we could never find yarn 2+ (since they don't allow(? encourage?) global installations).
  • This meant we were calling the wrong command. The -W is right for yarn 1, so we were trying to use it, even though the repo is berry.

I've tacked on a couple commits that I think should level out these behaviors and have hand tested this implementation on my machine. I'm going to ask a teammate to look this over because:

  1. These package-manager-binary-finding things are finnicky.
  2. The tests were passing before, and still are now. I believe this is because our tests don't run against real package manager installations, but haven't been able to confirm. I think I see it in the mocks but not certain.

anthonyshew avatar Dec 02 '24 05:12 anthonyshew

@anthonyshew awesome! let us know if we can help with anything else 🙌

tomcastro avatar Dec 02 '24 07:12 tomcastro