这是indexloc提供的服务,不要输入任何密码
Skip to content

fix: upgrade the turbo package in Yarn 4 #8076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 20, 2025

Conversation

atimmer
Copy link

@atimmer atimmer commented May 3, 2024

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 atimmer requested a review from a team as a code owner May 3, 2024 08:33
@atimmer atimmer requested review from Zertsov and NicholasLYang May 3, 2024 08:33
@turbo-orchestrator turbo-orchestrator bot added needs: triage New issues get this label. Remove it after triage owned-by: turborepo pkg: turbo-codemod labels May 3, 2024
Copy link

vercel bot commented May 3, 2024

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

Copy link

vercel bot commented May 3, 2024

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

A member of the Team first needs to authorize it.

@anthonyshew
Copy link
Contributor

anthonyshew commented Jun 15, 2024

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 anthonyshew removed the needs: triage New issues get this label. Remove it after triage label Jun 15, 2024
@turbo-orchestrator turbo-orchestrator bot added the needs: triage New issues get this label. Remove it after triage label Jun 15, 2024
@anthonyshew anthonyshew removed the needs: triage New issues get this label. Remove it after triage label Jun 16, 2024
@turbo-orchestrator turbo-orchestrator bot added the needs: triage New issues get this label. Remove it after triage label Jun 16, 2024
@anthonyshew anthonyshew removed the needs: triage New issues get this label. Remove it after triage label Jun 16, 2024
@turbo-orchestrator turbo-orchestrator bot added the needs: triage New issues get this label. Remove it after triage label Jun 16, 2024
@atimmer atimmer force-pushed the fix/upgrading-turbo-in-yarn4 branch from 0916755 to 6f393b3 Compare July 8, 2024 09:42
@atimmer
Copy link
Author

atimmer commented Jul 8, 2024

@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 😄

@tomcastro
Copy link

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?

@anthonyshew
Copy link
Contributor

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

@tomcastro
Copy link

@anthonyshew sure thing, created this PR

@@ -56,8 +56,8 @@ function getLocalUpgradeCommand({
`turbo@${to}`,
installType === "devDependencies" && "--dev",
]);
// yarn 1.x
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This confused me for a longer moment than I'd care to admit. I moved it down below the closing brace since it makes more sense there.

@anthonyshew
Copy link
Contributor

anthonyshew commented Dec 2, 2024

@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.

@tomcastro
Copy link

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

@anthonyshew anthonyshew merged commit c0a1dbb into vercel:main Jan 20, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: triage New issues get this label. Remove it after triage pkg: turbo-codemod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants