WP Build: avoid infinite worker build loop#80361
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: 0 B Total Size: 7.75 MB |
99046be to
48e42ce
Compare
| } | ||
|
|
||
| // Skip build-generated worker bundles written back into src/. | ||
| if ( relativePath.endsWith( '/src/worker-code.ts' ) ) { |
There was a problem hiding this comment.
How/where is this defined as an output?
(My adjacent thinking here, is that it feels that we're hard coding a lot of stuff here, in what is supposed to be a generic tool)
There was a problem hiding this comment.
The worker-code.ts file is produced during build when processing the wpWorkers key in package.json. The vips and video-conversion packages currently define a worker.
It's part of a pipeline that:
- Bundles the
wpWorkersentrypoint as a separate JS bundle. - Produces a
worker-code.tsfile that exports the JS bundle as a big string. - Imports the big string and creates a blob URL from it.
- Spawns a new web-worker with
new Worker( blobURL ). - Talks to the worker using a proxy object that implements methods as async message senders/receivers.
There was a problem hiding this comment.
I assumed it was one of those things where WP Build is just highly opinionated about things, but I'm not familiar with the Worker builder implementation or decisions; maybe @adamsilverstein is?
Probably a bit out of scope for this fix to change it, tho.
There was a problem hiding this comment.
Why is the produced worker-code.ts in "src"?
There was a problem hiding this comment.
I could meanwhile put the path to a shared constant, but a proper fix would indeed be moving out from src but I'm not really familiar with this setup to touch it too much now. :-)
There was a problem hiding this comment.
We have code in src that imports the worker-code.ts module and uses it to load the worker:
import { workerCode } from './worker-code';
const blobUrl = URL.createObjectURL( new Blob( [ workerCode ] ) );
new Worker( blobUrl );This ./worker-code import must resolve to something in all relevant build tasks. During TypeScript typechecking (tsc --build) and during esbuild build. If the worker-code.ts was outside src, how else would we import it? It would have to be a separate package or something.
We have a generate-worker-placeholders.mjs script that runs before TypeScript to generate a worker-code.ts placeholder file so that the tsc build that follows doesn't fail with unresolved modules. And then another build task generates the real files.
What @ramonjd tried in #76118 seems to be:
- Fool TypeScript with a mere
worker-code.d.tsfile that declares types and no code. - Adding an esbuild plugin that externalizes the
worker-code.tsimport, postponing resolution until both the importer and importee are both inbuild.
For some reason that didn't work, maybe TypeScript wasn't fooled? The CI logs for that March 20 build are expired, it can't be immediately checked.
For the record, this wpWorkers pipeline is not particularly elegant, we should simplify it. But at the moment I don't know why.
There was a problem hiding this comment.
I don't have much to add here other that some of the complexity/needing to encode the has to do with our limited control of the runtime environment (eg. we can't rely on setting headers).
For the record, this wpWorkers pipeline is not particularly elegant, we should simplify it.
I am open to reworking how we approach this. For now, adding an explicit path here to avoid the build recursion seems fine.
There was a problem hiding this comment.
For some reason that didn't work, maybe TypeScript wasn't fooled?
Good summary! And thanks for getting this PR up.
Yeah the general thrust of my tests was to make sure imports of ./worker-code were resolved without writing generated code into src/.
I think the CI was borking pretty hard so I closed it without comment (or my attention was diverted). I'll fire #76118 up again, rebase and just let it run to document it.
48e42ce to
3bd1d59
Compare
|
Flaky tests detected in c8a1116. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/29913425586
|
3bd1d59 to
ba0103f
Compare
ba0103f to
c8a1116
Compare
jsnajdr
left a comment
There was a problem hiding this comment.
Even if we decide to improve the wasm/worker build system in the future, this is still a good fix to have.
What?
Stop
wp-build --watchfrom rebuild-looping on generatedsrc/worker-code.tsfor worker packages (@wordpress/vips,@wordpress/video-conversion).Why?
Those packages write bundled worker/WASM output back into
src/worker-code.tson every build. The watcher treated that as a source change and rebuilt again (~3–5s each), spamming✅ vipsduringnpm run dev/storybook:dev/npm run storybook:e2e:dev.How?
Ignore
**/worker-code.tsin the package watcher, and skip it inisPackageSourceFileas a second guard.Path is hardcoded and used in WP Build initially here:
gutenberg/packages/wp-build/lib/worker-build.mjs
Line 117 in 5e5c9ea
Path is also already ignored by ESLint and Gitignore files around repo:
gutenberg/tools/eslint/config.mjs
Lines 252 to 253 in 5e5c9ea
gutenberg/packages/video-conversion/.gitignore
Lines 1 to 3 in 5e5c9ea
gutenberg/packages/vips/.gitignore
Lines 1 to 3 in 5e5c9ea
Testing Instructions
Not sure other than running
npm run storybook:devornpm run storybook:e2e:devfor a while. :-)Testing Instructions for Keyboard
Use of AI Tools
yes