Skip to content
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

Support creating patches with Yarn 2+ #506

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

noahm
Copy link

@noahm noahm commented Dec 20, 2023

Attempting to create new patch files with patch-package within a yarn 2+ project currently doesn't work. This has a few small changes to fixes all of them and support modern versions of yarn. Fixes: #496 #272

But first, why bother? Yarn 2 and up has its own built-in option for patching dependencies, so projects using it don't need patch-package anymore, right? On the contrary, patch-package not only has a much better overall user experience, but with the addition of support multiple patch files per package in v8.0.0, patch-package is a far more attractive solution for my needs. In particular, in one project there's a single critical dependency that I have made extensive customizations to over several years, and it's always a big effort to maintain those changes when they are combined into a single patch file. Yes, it's still technically possible to use yarn's native patch system to progressively layer patch files onto a single dependency, but it's a bit nightmarish. I would much prefer to continue using patch-package than jump through those types of hoops.

Now, for the issues I had to resolve to get patch-package with yarn 3 working:

  1. Problem: Initial tmp directory install fails because --ignore-engines option is not supported
    • Cause: yarn v3 bails if you pass--ignore-engines because support for that option is removed
    • Solution: only pass --ignore-engines if yarn version starts with 1.
  2. Problem: Our private repo config isn't being respected during the initial tmp directory install
    • Cause: makePatch.ts copies over any .yarnrc file it finds, but we use a .yarnrc.yml file
    • Solution: also copy .yarnrc.yml files into the tmp directory
  3. Problem: Installs in the tmp directory take a long time, and eventually fail, with lots of error spew about cache mismatch, install state issues, etc
    • Cause: makePatch.ts copies over the entire .yarn directory which includes a very large project-specific package cache, a state file for the current project's node modules, etc
    • Solution: only copy two specific subfolders of the .yarn directory: .yarn/releases which contains the currently installed version of yarn, and .yarn/plugins which contains any additional plugins (in our case the workspace tools plugin was required)

Finally, one additional "fix" that's not quite so objective is included. The error messaging produced by patch-package in the above failure cases was extremely hard to interpret, and doesn't include any sort of stack trace to help identify where the error is even coming from:

/Users/nmannesc/dev/twitch/twilight/node_modules/patch-package/dist/makePatch.js:395
        throw e;
        ^
{
  status: 1,
  signal: null,
  output: [
    null,
    Buffer(350) [Uint8Array] [
      121,  97, 114, 110,  32, 105, 110, 115, 116,  97, 108, 108,
       32, 118,  49,  46,  50,  50,  46,  49,  48,  10, 105, 110,
      102, 111,  32,  78, 111,  32, 108, 111,  99, 107, 102, 105,
      108, 101,  32, 102, 111, 117, 110, 100,  46,  10,  91,  49,
       47,  52,  93,  32,  82, 101, 115, 111, 108, 118, 105, 110,
      103,  32, 112,  97,  99, 107,  97, 103, 101, 115,  46,  46,
       46,  10, 105, 110, 102, 111,  32,  73, 102,  32, 121, 111,
      117,  32, 116, 104, 105, 110, 107,  32, 116, 104, 105, 115,
       32, 105, 115,  32,
      ... 250 more items
    ],
    Buffer(2018) [Uint8Array] [
      119,  97, 114, 110, 105, 110, 103,  32, 112,  97,  99, 107,
       97, 103, 101,  46, 106, 115, 111, 110,  58,  32,  78, 111,
       32, 108, 105,  99, 101, 110, 115, 101,  32, 102, 105, 101,
      108, 100,  10, 119,  97, 114, 110, 105, 110, 103,  32,  78,
      111,  32, 108, 105,  99, 101, 110, 115, 101,  32, 102, 105,
      101, 108, 100,  10, 119,  97, 114, 110, 105, 110, 103,  32,
       82, 101, 115, 111, 108, 117, 116, 105, 111, 110,  32, 102,
      105, 101, 108, 100,  32,  34, 112,  97, 116,  99, 104,  58,
      114, 101,  97,  99,
      ... 1918 more items
    ]
  ],
  pid: 3504,
  stdout: Buffer(350) [Uint8Array] [
    121,  97, 114, 110,  32, 105, 110, 115, 116,  97, 108, 108,
     32, 118,  49,  46,  50,  50,  46,  49,  48,  10, 105, 110,
    102, 111,  32,  78, 111,  32, 108, 111,  99, 107, 102, 105,
    108, 101,  32, 102, 111, 117, 110, 100,  46,  10,  91,  49,
     47,  52,  93,  32,  82, 101, 115, 111, 108, 118, 105, 110,
    103,  32, 112,  97,  99, 107,  97, 103, 101, 115,  46,  46,
     46,  10, 105, 110, 102, 111,  32,  73, 102,  32, 121, 111,
    117,  32, 116, 104, 105, 110, 107,  32, 116, 104, 105, 115,
     32, 105, 115,  32,
    ... 250 more items
  ],
  stderr: Buffer(2018) [Uint8Array] [
    119,  97, 114, 110, 105, 110, 103,  32, 112,  97,  99, 107,
     97, 103, 101,  46, 106, 115, 111, 110,  58,  32,  78, 111,
     32, 108, 105,  99, 101, 110, 115, 101,  32, 102, 105, 101,
    108, 100,  10, 119,  97, 114, 110, 105, 110, 103,  32,  78,
    111,  32, 108, 105,  99, 101, 110, 115, 101,  32, 102, 105,
    101, 108, 100,  10, 119,  97, 114, 110, 105, 110, 103,  32,
     82, 101, 115, 111, 108, 117, 116, 105, 111, 110,  32, 102,
    105, 101, 108, 100,  32,  34, 112,  97, 116,  99, 104,  58,
    114, 101,  97,  99,
    ... 1918 more items
  ],
  error: null
}

Node.js v20.5.1

In an attempt to at least get a useful stack trace on screen in error cases of this class, I've edited spanSafe.ts to create a new error object and attach it to the thrown object which provides a clear stack trace to the likely source of issues and it appears as one of the last things printed before the process exits:

/Users/nmannesc/dev/twitch/twilight/node_modules/patch-package/dist/makePatch.js:395
        throw e;
        ^
{
  status: 1,
  signal: null,
  output: [
    null,
    Buffer(351) [Uint8Array] [
      121,  97, 114, 110,  32, 105, 110, 115, 116,  97, 108, 108,
       32, 118,  49,  46,  50,  50,  46,  49,  48,  10, 105, 110,
      102, 111,  32,  78, 111,  32, 108, 111,  99, 107, 102, 105,
      108, 101,  32, 102, 111, 117, 110, 100,  46,  10,  91,  49,
       47,  52,  93,  32,  82, 101, 115, 111, 108, 118, 105, 110,
      103,  32, 112,  97,  99, 107,  97, 103, 101, 115,  46,  46,
       46,  10, 105, 110, 102, 111,  32,  73, 102,  32, 121, 111,
      117,  32, 116, 104, 105, 110, 107,  32, 116, 104, 105, 115,
       32, 105, 115,  32,
      ... 251 more items
    ],
    Buffer(2018) [Uint8Array] [
      119,  97, 114, 110, 105, 110, 103,  32, 112,  97,  99, 107,
       97, 103, 101,  46, 106, 115, 111, 110,  58,  32,  78, 111,
       32, 108, 105,  99, 101, 110, 115, 101,  32, 102, 105, 101,
      108, 100,  10, 119,  97, 114, 110, 105, 110, 103,  32,  78,
      111,  32, 108, 105,  99, 101, 110, 115, 101,  32, 102, 105,
      101, 108, 100,  10, 119,  97, 114, 110, 105, 110, 103,  32,
       82, 101, 115, 111, 108, 117, 116, 105, 111, 110,  32, 102,
      105, 101, 108, 100,  32,  34, 112,  97, 116,  99, 104,  58,
      114, 101,  97,  99,
      ... 1918 more items
    ]
  ],
  pid: 16211,
  stdout: Buffer(351) [Uint8Array] [
    121,  97, 114, 110,  32, 105, 110, 115, 116,  97, 108, 108,
     32, 118,  49,  46,  50,  50,  46,  49,  48,  10, 105, 110,
    102, 111,  32,  78, 111,  32, 108, 111,  99, 107, 102, 105,
    108, 101,  32, 102, 111, 117, 110, 100,  46,  10,  91,  49,
     47,  52,  93,  32,  82, 101, 115, 111, 108, 118, 105, 110,
    103,  32, 112,  97,  99, 107,  97, 103, 101, 115,  46,  46,
     46,  10, 105, 110, 102, 111,  32,  73, 102,  32, 121, 111,
    117,  32, 116, 104, 105, 110, 107,  32, 116, 104, 105, 115,
     32, 105, 115,  32,
    ... 251 more items
  ],
  stderr: Buffer(2018) [Uint8Array] [
    119,  97, 114, 110, 105, 110, 103,  32, 112,  97,  99, 107,
     97, 103, 101,  46, 106, 115, 111, 110,  58,  32,  78, 111,
     32, 108, 105,  99, 101, 110, 115, 101,  32, 102, 105, 101,
    108, 100,  10, 119,  97, 114, 110, 105, 110, 103,  32,  78,
    111,  32, 108, 105,  99, 101, 110, 115, 101,  32, 102, 105,
    101, 108, 100,  10, 119,  97, 114, 110, 105, 110, 103,  32,
     82, 101, 115, 111, 108, 117, 116, 105, 111, 110,  32, 102,
    105, 101, 108, 100,  32,  34, 112,  97, 116,  99, 104,  58,
    114, 101,  97,  99,
    ... 1918 more items
  ],
  error: Error: command exited with non-zero status
      at Object.spawnSafeSync (/Users/nmannesc/dev/twitch/twilight/node_modules/patch-package/dist/spawnSafe.js:23:32)
      at Object.makePatch (/Users/nmannesc/dev/twitch/twilight/node_modules/patch-package/dist/makePatch.js:131:29)
      at /Users/nmannesc/dev/twitch/twilight/node_modules/patch-package/dist/index.js:72:25
      at Array.forEach (<anonymous>)
      at Object.<anonymous> (/Users/nmannesc/dev/twitch/twilight/node_modules/patch-package/dist/index.js:71:22)
      at Module._compile (node:internal/modules/cjs/loader:1233:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1287:10)
      at Module.load (node:internal/modules/cjs/loader:1091:32)
      at Module._load (node:internal/modules/cjs/loader:938:12)
      at Module.require (node:internal/modules/cjs/loader:1115:19)
}

Node.js v20.5.1

I haven't familiarized myself enough with the integration test setup in this project to build this PR out into something that I would consider ready to merge, but I think it should be a good starting point. And, hopefully my intro above is a convincing case for why modern yarn versions are still worth supporting in this package.

Comment on lines +217 to +220
const yarnVersionCmd = spawnSafeSync(`yarn`, ["--version"], {
cwd: tmpRepoNpmRoot,
logStdErrOnError: false,
})
Copy link
Author

Choose a reason for hiding this comment

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

Is there a more elegant way to interrogate the currently version of yarn?

@@ -332,7 +345,6 @@ export function makePatch({
)
}
process.exit(1)
return
Copy link
Author

Choose a reason for hiding this comment

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

These returns are dead code, as process.exit ends the process immediately and does not return

@JounQin
Copy link

JounQin commented Dec 21, 2023

After using this patch in our internal CI, I believe this PR is quite unfinished, I'll create a new PR based yours. For example, packageManager, --mode=skip-build support, and resolve resolution for the following correctly:

"@backstage/integration@npm:^1.5.0, @backstage/integration@npm:^1.7.0, @backstage/integration@npm:^1.7.2":
  version: 1.7.2
  resolution: "@backstage/integration@npm:1.7.2"

@noahm See #507

JounQin added a commit to un-ts/patch-package that referenced this pull request Dec 21, 2023
JounQin added a commit to un-ts/patch-package that referenced this pull request Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for yarn v2+
2 participants