-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Compat: Refactor patch generation scripts #7009
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
base: master
Are you sure you want to change the base?
Conversation
Also make sure checks are run for updates to the patch scripts, not just the patches themselves
7377690 to
7c4e326
Compare
| synchronizeProgram(); | ||
| diff --git a/lib/tsserver.js b/lib/tsserver.js | ||
| index 20cb517f8..eeac46d44 100644 | ||
| index 20cb..eeac 100644 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it expected that those files changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's part of unifying git diff options. The 3 packages were using different hash lengths for the index header. fsevents was using 40, typescript was using 9. resolve was using the default which I think is 8 but the docs does not say and I don't know if something can affect that or what.
So I wanted to explicitly pass the --abbrev option. But git apply doesn't check the hashes, and I don't think we do either. So if we are explicit anyway, why not choose the minimum (which is 4)?
| for (const slice of slices) { | ||
| this.getValidateVersions(slice) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit for readability: I'd prefer a Promise.all(slices.map(...)), and to separate it in two passes (first call getValidateVersions on all slices, aggregate that in an array, and then make another Promise.all on all the versions you got this way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
Theoretically, it does have a perf impact as the fastest getValidateVesrions will be blocked by the slowest. But practically, the only truly async getValidateVesrions is the TypeScipt one and they are all waiting on the TypeScipt version fetch anyway.
|
I was thinking adding a fallback to building TypeScript with Corepack is safe because the CI process would build the patch with Volta and check. Turns out the job doesn't have Volta and has been checking only the aggregation of patches to bundle. I want the CI process to check by fully regenerating the patch bundles. But building the TypeScript patch takes quite a long time that other checks shouldn't have to wait for it. To that end, I think we should move the check into the plugin-compat workflow as a new job where we install Volta and run the check. I'll implement that later. |
What's the problem this PR addresses?
I was planning to update the built-in TypeScript patch when I encountered some inconveniences working with the built-in patches:
typescriptscript uses POSIX paths, andgit diffgenerates patches with quoted Windows paths which the script can't work withyarnpkg/TypeScriptfor generating patchesgit diffoptions, which may cause inconsistencies and the generated patch may be affected by the contributor's global git config.How did you fix it?
Reimplemented all 3 patch generation scripts in TypeScript, using
@yarnpkg/coreand@yarnpkg/fslibutilities to make them cross-platform.The common parts of the generation procedure are extracted into a shared module, ensuring the patches are generated with the same options and ignoring the contributor's global git config as much as possible. Also,
git diffis executed in a way that does not incorporate Windows paths into the patch.This new patch generator includes documentation on usage and maintenance via JSDoc comments and the README.
In case someone wants to review how the patches are changed during this rewrite, the first 3 commits in this PR provides a step-by-step view:
fseventspatches have an extraenous/copy/in the source pathresolvesources are updated in Upgrades Eslint to v9 #6694 but the patch was nottypescriptthat applies to no versions of TypeScriptgit diffoptions are preserved, to minimize the change to the patches in this commitgit diffoptions are unifiedAlso made the CI process check for consistency of the patches when anything inside
packages/plugin-compat/extrachanges, not just when the bundled patches themselves change.For the TypeScript patch, I added a fallback to using Corepack if Volta is not installed. A warning is printed in this case. While Corepack only switches npm versions, not node versions, I tested it on node 22.21.1 and all patches are built without changes. And the CI checks the patch using Volta so there is little risk of publishing a bad patch.
On the output, I tried to find a async task runner library to manage that.
tasukuseemed promising at first but output that is not managed via it will be moved to above the task list, even after all tasks are complete. In the end, I created a very rudimentary logger for the scripts.Checklist