Makes patches optional #2543
Makes patches optional #2543
Conversation
| // changeset. We need to open it for each patchfile (rather than only a | ||
| // single time) because it lets us easily rollback when hitting errors | ||
| // on optional patches (we just need to call `discardAndClose`). | ||
| const patchedPackage = new ZipFS(currentFile, { |
There was a problem hiding this comment.
Could you change this part and the initialCopy to be in-memory, then write the resulting buffer to disk for the returned ZipFS instance?
There was a problem hiding this comment.
I don't think I can, since we need to take snapshots in-between each patch. If I was to closeAndSave a memory zipFs, then we'd to already have in memory another one representing the last snapshot; this would be difficult unless we clone the memory zipFs somehow, which I don't know how to do efficiently.
There was a problem hiding this comment.
@arcanis Wouldn't this work? mael/optional-patches...paul/patch-example
There was a problem hiding this comment.
Oh I forgot getBufferAndClose 🤔 Still, it would require to keep the whole archive in memory, twice per package. I'm a bit worried it would be heavy (by contrast, by closing / opening, it only needs the archive listing)... given that the general case is to apply a single patch, I'd tend to first try the fs approach and switch only if we see a noticeable impact.
There was a problem hiding this comment.
Can we at least profile both approaches and compare the time spent and the memory usage to make sure that we don't introduce a significant performance regression? Since compression is the slowest part of the fetch step, I'd prefer to avoid having to compress the archive for each patch file. I've also just realized that my changes still aren't optimal because they don't prevent compression (I have to pass level: 0 for that to happen).
There was a problem hiding this comment.
Sure, will check - I however expect the compression level to affect new writes, not the existing ones. With that in mind we can't disable compression, since it would require to go back and compress the affected files after all the patches have been applied - far too complex for a fringe use case (multiple patches on the same package).
There was a problem hiding this comment.
For the record: 22s install time for TS code cache, vs 15s before.
|
Going to merge it now; seems like the patch will break again with TS 4.3 Beta 🙁 In any case we'll likely have to release a 2.4.2 just for this. |
Core problem is that `yarn berry`, when using `pnp` mode, requires
a patch to `typescript`. But we're using the `node-modules` linker.
Need to upgrade yarn I think
Tracking issues:
- yarnpkg/berry#2935
- yarnpkg/berry#2543
- yarnpkg/berry#2711
Core problem is that `yarn berry`, when using `pnp` mode, requires
a patch to `typescript`. But we're using the `node-modules` linker.
Need to upgrade yarn I think
Tracking issues:
- yarnpkg/berry#2935
- yarnpkg/berry#2543
- yarnpkg/berry#2711
What's the problem this PR addresses?
The builtin patches are currently problematic as they prevent users from using the latest versions of some dependencies (in particular TS), with no real workaround (#2514).
How did you fix it?
This diff implements a syntax making patches optional, and turns it on for the builtin compatibility patch. If such patches fail to cleanly apply, they'll simply fallback to the original sources rather than break. Users will then be able to temporarily enable the node_modules linker to circumvent the problem.
Why not just disable patches with nm linkers?
Checklist