Reimplement @embroider/webpack on the modern (vite-style) architecture - #2729
Reimplement @embroider/webpack on the modern (vite-style) architecture#2729NullVoxPopuli wants to merge 7 commits into
Conversation
#10) * Reimplement @embroider/webpack on the modern (vite-style) architecture Rewrites @embroider/webpack from scratch so it mirrors @embroider/vite instead of the legacy stage3 Packager pipeline. The app keeps its real index.html/tests/index.html, the compat prebuild produces the .embroider working dir, and webpack bundles using @embroider/core's Resolver + virtualContent. - ember-webpack.ts: emberWebpack() config factory (analog of vite's ember()+classicEmberSupport()), returned as a config function so --mode is honored. - webpack-resolver-plugin.ts: modern EmbroiderPlugin, lazy resolver.json load, threads the full VirtualResponse to the virtual loader. - virtual-loader.ts: implemented (was a fixme stub) via (de)serialized VirtualResponse + virtualContent. - html-output-plugin.ts: HTML facade — content-for substitution, inline module externalization, import.meta.glob -> require.context, raw virtual asset emission (vendor/test-support), final HTML rewrite. - compat-prebuild.ts, assets-plugin.ts, build-once.ts, template-tag-loader.ts: analogs of vite's compatPrebuild/assets/buildOnce/templateTag. - Drop legacy Packager + thread-loader/csso/lodash/semver/supports-color. - Package is now a composite TS project emitting dist/ (like @embroider/vite); root tsconfig references it. - test-setup no longer depends on the removed legacy Webpack packager type. - Add tests/scenarios/webpack-app-test.ts and webpack-cli dev dep. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * webpack: make emberWebpack() a config-mutating plugin (review feedback) Per review: don't own the whole webpack Configuration. emberWebpack() now returns a webpack plugin that mutates compiler.options (resolve, module.rules, resolveLoader, output, optimization, experiments) and sets an async entry that runs the compat prebuild before discovering the html entrypoints — mirroring how the vite plugins mutate vite's config via config(). Apps now use a normal webpack.config.js: `module.exports = { plugins: [emberWebpack()] }`. Dropped the unused Options.webpackConfig. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * webpack: split into classicEmberSupport() + ember() plugins (review feedback) Mirrors @embroider/vite's two-plugin API. classicEmberSupport() does the compat prebuild, content-for substitution, v2 addon public-assets and the .hbs rule; ember() does the resolver, the template-tag/babel/css rules, the build config mutation and the html entrypoints. They coordinate through one per-compiler Shared object. App webpack.config.js is now: `module.exports = { plugins: [classicEmberSupport(), ember()] }`. Also fixes the preflight lint failure: removed an unused eslint-disable directive in assets-plugin.ts (CI runs --report-unused-disable-directives). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * webpack: expand webpack-app scenario across the full Ember matrix Use wideAppScenarios (fullSupportMatrix) just like vite-app-basics, so webpack-app-basics produces <emberVersion>-webpack-app-basics for the same set of Ember versions (lts_3_28 ... canary) that vite is expanded into. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * webpack: fix singleton double-include (one webpack entry per html page) CI showed lts_3_28..lts_5_4 webpack-app-basics failing at runtime with "@glimmer/validator included twice" (and, in a regressed experiment, "QUnit already defined"). Root cause: each inline <script type="module"> in index.html / tests/index.html was turned into its own webpack entrypoint. Webpack gives each entrypoint its own runtime + module registry, so a module shared between the entrypoints on a page (singletons like @glimmer/validator, which older ember-source imports from many places, or qunit) was instantiated once per entrypoint -> "included twice". Production passed because it only has the single index.html entry; rollup/vite never hit this because a build has one module registry. Fix: emit one webpack entry per html page, with all of that page's module scripts as ordered `import`s (matching how rollup/vite treat a document). The first module-script placeholder receives the page's built assets; subsequent ones are removed (their code is already in the page entry, in order). Also adopt vite's id-based virtual-module model: the loader request is keyed only by the stable virtual specifier and the VirtualResponse is handed across in-process via a registry (like vite's responseMetas + load(id)), instead of serializing an importer-dependent payload into the request. Validated locally: lts_3_28, lts_5_4 and release webpack-app-basics all green for both `pnpm build` (production) and `pnpm test:ember` (dev + ember test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
NullVoxPopuli-ai-agent
left a comment
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Context: I'm the agent that wrote this rewrite. Posting the non-obvious caveats/problems and how they were solved — most are inline on the relevant lines; this summary covers the cross-cutting bits.
Dead-ends investigated for the "@glimmer/validator included twice" CI failure (≤ lts_5_4)
Tried and rejected (all reverted; none are in the final diff):
resolve.symlinks: false— embroider-conventional, but no effect here.cleanUrl(fromFile)in the resolver adapter to mirror rollup's importer query-stripping — plausible (the fastboot-switch specifier carries?names=…), but no effect; reverted.optimization.runtimeChunk: 'single'— dedupes the registry across entrypoints, but didn't fix the older versions and regressedreleasewith "QUnit already defined" (it globally couples unrelated pages' entrypoints). Reverted.
The actual root cause and fix are documented inline on html-output-plugin.ts (one webpack entry per html page). The @glimmer/validator.js? in the stack traces was a red herring — webpack's loader query in the sourceURL, not a second module; a module dump proved there was always exactly one webpack module. The duplication was across per-entrypoint runtimes.
Limitations / not covered (intentional scope)
- Fastboot variant not implemented.
experiments.topLevelAwaitis kept to leave room, but there's no fastboot scenario or fastboot-switch runtime wiring beyond what the core resolver provides. - No dev-server equivalent to
vite dev— this iscompatBuild(app, buildOnce)+webpack buildonly. - Only the standard
app-templatewiring is provided/validated;ts-app-template,app-template-minimal,ts-app-template-classicare not converted to a webpack variant. - The
import.meta.glob→require.contexttransform only handles the eager forms the templates use.
Unrelated CI noise
lts_6_12-typescript-app and release-compat-template-colocation-…-windows failed on Chrome/dbus infra flakes in scenarios that don't use @embroider/webpack — not caused by this PR.
Architecture note
Throughout, the guiding principle was "do what @embroider/vite does": two config-mutating plugins (classicEmberSupport() + ember()), id-based virtual modules, raw-emit of vendor/test-support, public-assets for app.css. Where webpack's model differs from rollup's (per-runtime module registry, no HTML/import.meta.glob handling, no buildEnd emit, request-string module identity) is exactly where the inline comments are.
| ], | ||
| "extends": "../../tsconfig-base.json", | ||
| "compilerOptions": { | ||
| "composite": true, |
There was a problem hiding this comment.
Caveat I hit: this package had to become a composite TS project emitting dist/ (mirroring @embroider/vite). The legacy layout compiled .js next to .ts with no tsconfig. The moment there's an index.ts, tsc -b emits index.d.ts next to it, which is then also matched as an input by the root tsconfig.json include glob -> error TS5055: Cannot write file ... it would overwrite input file, intermittently (only once stale artifacts exist). Composite + outDir: dist, plus referencing/excluding this project in the root tsconfig, was the fix.
| "license": "MIT", | ||
| "author": "Edward Faulkner", | ||
| "main": "src/ember-webpack.js", | ||
| "main": "dist/src/index.js", |
There was a problem hiding this comment.
Consequence of the composite build: main/types point at dist/src/*. Bumped to 5.0.0 (from-scratch rewrite; legacy Packager removed). Dropped thread-loader/csso/lodash/semver/supports-color. thread-loader removal is load-bearing: see the virtual-module registry note on webpack-resolver-plugin.ts — loaders now always run in-process, which is what makes the resolver↔loader registry valid (like vite's in-memory responseMetas).
| // .embroider working directory (resolver.json, content-for.json, rewritten | ||
| // packages) plus the prebuilt vendor/test-support assets into | ||
| // tmp/compat-prebuild. | ||
| let prebuildPromise: Promise<void> | undefined; |
There was a problem hiding this comment.
Problem: resolver.json / content-for.json only exist after the compat prebuild, but webpack plugins are applied before the build runs. Solution: a process-cached prebuild promise. The plugin taps beforeRun/watchRun only to sequence logs; the real ordering guarantee is that ember()'s async entry awaits this same cached promise before discovering anything.
| const { entrypoints, publicAssetURL, packageName, resolverConfig } = appInfo; | ||
| // classicEmberSupport() and ember() coordinate through one Shared object per | ||
| // compiler, regardless of the order they appear in the plugins array. | ||
| const sharedByCompiler = new WeakMap<Compiler, Shared>(); |
There was a problem hiding this comment.
classicEmberSupport() and ember() need to coordinate (content-for flag, whether to run the prebuild) but webpack applies plugins independently and in arbitrary array order. Solution: a single Shared object per Compiler via a WeakMap; whichever plugin runs first creates it. This is the webpack stand-in for the way vite's plugins just share process.cwd()/module state.
| // embroider owns entry. We set it to an async function (webpack supports | ||
| // this) so the compat prebuild has produced resolver.json / | ||
| // content-for.json before we discover the html entrypoints. | ||
| opts.entry = (async () => { |
There was a problem hiding this comment.
entry is an async function (webpack supports this) that runs the prebuild and then discovers html entrypoints. I first tried adding entries dynamically with EntryPlugin in the make hook; that fails with "No dependency factory available for dependency type: EntryDependency" because only EntryPlugin registers that factory, at thisCompilation, which is too late when you call compilation.addEntry from make. The async entry fn avoids the whole problem.
| // Hand the VirtualResponse across in-process and key the request only by | ||
| // the stable specifier, so the same logical virtual always produces the | ||
| // same webpack module (this is what rollup/vite get for free). | ||
| registerVirtualResponse(this.appRoot, virtual); |
There was a problem hiding this comment.
Adopted vite's id-based virtual-module model here. Webpack keys modules by the full request string, so encoding the (importer-dependent) VirtualResponse into the request made the same logical virtual (e.g. a fastboot switch) produce multiple webpack modules. Now the request carries only the stable virtual.specifier; the VirtualResponse is handed across in-process via a registry in virtual-loader.ts, exactly like vite's responseMetas + load(id). This is only safe because thread-loader was dropped (loaders run in the same process as this plugin).
|
|
||
| constructor(opts: EmbroiderResolverOptions, babelLoaderPrefix: string) { | ||
| this.#resolver = new EmbroiderResolver(opts); | ||
| // Note: the resolver config (resolver.json) is read lazily, the first time |
There was a problem hiding this comment.
The embroider resolver is constructed lazily (first resolve), not in the constructor: resolver.json is written by the compat prebuild during the build, after this plugin has been apply'd.
|
|
||
| // A tiny RequestAdapter used to ask the core resolver "if the app imported | ||
| // this specifier, what virtual response would you produce?". This is the | ||
| // webpack analog of vite's `ensureVirtualResolve`, used so we can emit the |
There was a problem hiding this comment.
This tiny RequestAdapter exists because webpack has no buildEnd-style "emit these virtual files" hook like vite's resolver plugin. It asks the core resolver "what VirtualResponse would this specifier produce" so we can emit vendor/test-support as real assets in processAssets — the webpack-shaped equivalent of vite's emitVirtualFile.
| export function applyContentFor(html: string, htmlPath: string, appRoot: string): string { | ||
| let key = htmlPath.startsWith('/') ? htmlPath : '/' + htmlPath; | ||
| let configPath = join(locateEmbroiderWorkingDir(appRoot), 'content-for.json'); | ||
| if (!existsSync(configPath)) { |
There was a problem hiding this comment.
Tolerate a missing content-for.json: a fully-v2 app using only ember() (no classicEmberSupport()) never runs the compat prebuild, so there's nothing to substitute. Without this guard it threw ENOENT for that valid configuration.
| // Expanded across the same Ember matrix as `vite-app-basics` | ||
| // (wideAppScenarios / fullSupportMatrix), producing | ||
| // `<emberVersion>-webpack-app-basics`. | ||
| wideAppScenarios |
There was a problem hiding this comment.
Uses wideAppScenarios (fullSupportMatrix) so webpack-app-basics expands across the exact same Ember matrix as vite-app-basics (lts_3_28 … canary), each becoming its own CI job. Validation caveat: locally I only ran lts_3_28 + lts_5_4 + release × (pnpm build prod and pnpm test:ember dev+ember-test) — lts_3_28/lts_5_4 were the worst CI failures, so they're the highest-signal; the full matrix is delegated to CI.
…ixes cancelled CI) (#11) * webpack: reap the ember build --watch tree so `webpack serve` shuts down CI: every ubuntu `*-webpack-app-basics` job was cancelled (run conclusion "cancelled"). The job log shows all three tests pass (`ok 1/2/3`) and then `The operation was canceled` + `Terminate orphan process: pid (node)/(sh)/ (node)` — the job hung after the suite passed. Cause: `CommandWatcher.shutdown()` SIGINTs `webpack serve` and *awaits its exit*. webpack-dev-server's shutdown runs `compiler.close()` -> our `stopCompatPrebuild()` -> `watchChild.kill()`, but that only SIGTERMs the immediate `ember` node process. ember-cli's `--watch` forks broccoli / sane / a shell; those grandchildren were orphaned, the dev server never finished closing, `waitForExit()` blocked, and CI killed the job (whole run -> cancelled). Windows variants pass because the watch module is excluded there. Fix: spawn `ember build --watch` `detached` (its own process group) and, on stop, `process.kill(-pid, 'SIGKILL')` the whole group so teardown is immediate and never blocks the dev server. Also register process exit / SIGINT / SIGTERM / SIGHUP handlers so the tree is reaped even if webpack's shutdown hooks don't run. Validated locally: release-webpack-app-basics 3/3 green, qunit exits cleanly (exit 0), and zero leftover ember/webpack watch processes after the run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: clear CommandWatcher.waitFor timeout (fixes hung/cancelled CI) This is the actual cause of the cancelled `*-webpack-app-basics` CI jobs. `CommandWatcher.waitFor()` created `setTimeout(reject, timeout)` for its timeout race but never stored or cleared the handle. When the awaited output was found, the timer kept running as a ref'd libuv handle and pinned the Node event loop open for the *entire* timeout duration. The webpack scenario's `webpack dev` module does `server.waitFor(/Loopback:/, 10 * 60 * 1000)`, so after the suite passed (`# pass 3 # fail 0`) the qunit process couldn't exit for ~8-10 minutes. CI's job timeout fired first -> "The operation was canceled" -> every ubuntu `*-webpack-app-basics` job `cancelled` -> whole run `cancelled`. vite-app-test never hit this because its `vite dev` uses the 90s DEFAULT_TIMEOUT, short enough to exit before the job times out. Fix: keep the timer handle, `clearTimeout` it in a `finally`, and `.unref()` it belt-and-suspenders. Reproduced locally under CI=true: before, qunit hung >8min after `# fail 0`; after, it exits immediately. release-webpack-app-basics 3/3 green, clean prompt exit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… it needs (#12) * webpack: support the fully-v2 (ember()-only, no compat) path Enables @embroider/webpack for a pure-v2 app (no compat prebuild, no resolver.json, no content-for) — mirroring @embroider/vite's minimal path: - webpack-resolver-plugin: on `not_found`, fall back to webpack's default resolve (mirrors vite's `return null`) instead of hard-erroring, so plain relative app imports (./src/app) resolve when there's no resolver.json. - ember-webpack: add a module rule with `resolve.fullySpecified: false` so a `"type": "module"` app's extensionless imports resolve (top-level resolve.fullySpecified can't override per-ESM-module strictness). - ember-webpack: align process.env.NODE_ENV with the build mode so @embroider/macros' buildMacros() enables runtime mode (setTesting / runtime isTesting), which vite gets for free. - html-output-plugin: emit `import.meta.webpackContext` (not `require.context`) from the import.meta.glob transform — CJS `require` doesn't exist in an ESM ("type": "module") app. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * tests: add webpack "minimal" (pure-v2) scenario Mirror of minimal-app-test.ts for webpack: a dedicated app-template-webpack-minimal (fully-v2, webpack.config.cjs uses ember() only) + webpack-minimal-app-test.ts + webpackMinimalAppScenarios. Known gap (documented in the PR): under `.only('canary')`, updateEmberQunit() overwrites tests/test-helper.js with a variant that drops enterTestMode() and imports <pkg>/config/environment. vite satisfies that test-aware config for a pure-v2 app; the webpack ember()-only path does not yet, so the canary run currently reaches `# pass 4 # fail 3` (UnrecognizedURLError / isTesting false). The pure-v2 build + ember test otherwise pass on the template's pinned ember-source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * tests: don't impose classic config/environment test-helper on v2 apps The webpack (and vite) "minimal" canary failure was caused by updateEmberQunit() unconditionally overwriting tests/test-helper.js with the classic `<modulePrefix>/config/environment` + no-enterTestMode variant. A fully-v2 app defines its config in `#config`/src and ships its own correct test-helper (which calls enterTestMode()); the rewrite is wrong for that case. Skip it when `ember-addon.version === 2`. Also drop the `config/environment` re-export + exports entry I'd added to app-template-webpack-minimal as a workaround — the minimal app must not import `<modulePrefix>/config/environment` at all; it now mirrors app-template-minimal's exports/imports exactly. Result: canary-webpack-minimal-app passes (2/2); vite canary-minimal-app still passes (4/4) — no regression from the shared updateEmberQunit change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| { | ||
| "name": "@embroider/webpack", | ||
| "version": "4.1.0", | ||
| "version": "5.0.0", |
There was a problem hiding this comment.
bad claude, don't change this because it's using release-plan
3 participants
like how we have
@embroider/viteset up, but with webpack's constraintsTested myself locally
Warning
I don't know anything about webpack's APIs, other than them being quite unergonomic, and a pain to debug. Most of this PR was generated by claude. I suspect this PR has a lot more work left.