Speed up loader transpilation ~10x via hand-rolled AMD wrapper (CS-10977)#4573
Open
Speed up loader transpilation ~10x via hand-rolled AMD wrapper (CS-10977)#4573
Conversation
Replaces the per-module-load `transformAsync(..., TransformModulesAmdPlugin)`
call in `Loader.fetchModule` with a hand-rolled acorn + magic-string
transpiler in a new `packages/runtime-common/amd-transpile.ts`. The
realm-server has already lowered TS, decorators, glimmer templates, and
scoped CSS by the time the loader sees the source — so the loader's
babel pass was paying babel's full plugin pipeline cost for what is
effectively just an `import`/`export` rewrite + AMD wrapper.
Bench results vs current babel pipeline (median wall time on real card
fixtures, post-realm-server-transpile):
enum.js (10 KB): 23 ms -> 1.4 ms (16.8x)
spec.js (61 KB): 62 ms -> 3.8 ms (16.4x)
skill-set.js (71 KB): 58 ms -> 4.2 ms (13.9x)
card-api.js (118 KB): 352 ms -> 20 ms (18.0x)
SWC native (`module.type: 'amd'`) was also benchmarked and came in at
6.5-11.8x — the hand-rolled approach beats it because for these inputs
the Node<->Rust FFI marshalling cost dominates SWC, and we skip code
generation entirely by string-editing instead of re-printing an AST.
Live-binding semantics are preserved across module boundaries (re-
exports use `Object.defineProperty(...{ get })`); local source-code
uses of imported names are bound via `let` destructuring at the AMD
body entry — a snapshot, but unobservable for typical card code which
exports immutable classes/functions/consts. See the file header for
the full semantics rationale.
Includes 23 shared qunit unit tests covering imports, exports,
re-exports, `export *`, `import.meta`, and live-binding cases.
Validated functionally equivalent to babel on four real card fixtures
(card-api, enum, skill-set, spec): identical dep-set sizes and
identical export name sets.
CS-10977
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d566c69ce6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces the loader’s Babel-based ES module → AMD wrapping step with a hand-rolled acorn + magic-string transpiler to significantly reduce per-module load time during indexing and runtime module fetches.
Changes:
- Add a new
transpileAmd()implementation (acorn parse + magic-string edits) that emitsdefine(moduleId, deps, factory)with getter-based live bindings for re-exports. - Update
Loader.fetchModuleto usetranspileAmdinstead of@babel/core+transform-modules-amd-plugin. - Add shared QUnit tests for AMD wrapping behavior and hook them into realm-server’s test suite; add direct deps (
acorn,acorn-walk,magic-string) toruntime-common.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Adds acorn / acorn-walk to the workspace catalog for direct consumption. |
| pnpm-lock.yaml | Locks new catalog entries / versions for acorn-related deps and magic-string. |
| packages/runtime-common/amd-transpile.ts | New hand-rolled ES→AMD wrapper/transpiler used by the loader. |
| packages/runtime-common/loader.ts | Switch loader module wrapping from Babel to transpileAmd. |
| packages/runtime-common/package.json | Declares new direct dependencies used by amd-transpile.ts. |
| packages/runtime-common/tests/amd-transpile-test.ts | Adds shared unit tests validating wrapping and (claimed) live-binding behavior. |
| packages/realm-server/tests/index.ts | Registers the new amd-transpile test module in realm-server test entrypoint. |
| packages/realm-server/tests/amd-transpile-test.ts | Realm-server QUnit wrapper that runs the shared runtime-common tests. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
habdelra
changed the title
Six issues surfaced by CI failures + reviewer comments (Codex + Copilot)
on the original snapshot-based approach:
1. Circular dependencies: the `let { x } = _foo` snapshot at body entry
captured `undefined` whenever the dep had not yet finished evaluating
its body (which is exactly what happens for circular imports —
card-api.gts <-> contains-many-component.gts and friends). Symptom
on base-realm: every card instance failed to index with `Cannot read
properties of undefined (reading 'includes')`.
Replaced the snapshot with a scope-aware identifier-rewrite walk:
every non-shadowed source-code reference to an imported name is
rewritten to `_dep.name`. Lookup happens at use time, so by the time
a function references `field(...)` the dep has finished evaluating.
Matches babel's plugin behaviour.
2. `export default <expr>` magic-string overlap. The previous code
overwrote `node.start..node.end` with `_exports.default = <expr>`,
then the identifier-rewrite walk visited the same identifier inside
the same range -> magic-string `Cannot split a chunk that has
already been edited` -> render timed out at 90s. Symptom on base-
realm: `ai-app-generator.json` instance hung forever.
Now strip just the `export default ` prefix and append a
`var __default$N = (<expr>);` capture, with the
`_exports.default = __default$N` setter deferred to body end. The
expression source stays at its original AST positions for the
walker, no overlap, and forward-reference TDZ behaviour is
preserved.
3. Order-dependent re-export wiring. `export { x }` of a binding
imported AFTER it (legal ESM) silently downgraded to a snapshot
assignment instead of a live-binding getter. Fixed by making pass 1
collect every `ImportDeclaration` binding before pass 2 emits
exports.
4. Destructured exports threw. `export const { a, b } = obj` and
`export const [x, y] = arr` are valid ESM, but the previous code
threw a "unsupported destructuring pattern" error. Now walks the
pattern's binding identifiers and emits a getter per name.
5. `_default` name collisions. The hard-coded `_default` temp could
shadow a real top-level binding. Now uses `__default$N` where N is
the smallest integer that avoids every name declared at the module
level.
6. Mutable `export let` lost mutations after body init. `export let
counter = 0; export function inc() { counter++; }` -> importers
never saw the post-mutation value because the export was a
snapshot. Now installs a getter on `_exports.counter` so reads go
through the live local binding.
Plus: re-added `//# sourceURL=<moduleId>` to the `eval(src)` call in
`Loader.fetchModule` so stack traces from inside an AMD module name
the original module.
Validated end-to-end: a live realm-server stack using the new code
indexed the base realm with 0 file errors and 0 instance errors in
6.5s.
Bench impact: the scope-aware walk costs ~5 percentage points of the
original snapshot approach's speedup. Net is now ~9-12x over babel
across real card fixtures (vs ~15x for the broken snapshot, vs
6-12x for SWC native). Still clearly ahead of babel, no new native
deps.
Adds 6 regression tests covering each of the bugs above.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces the loader’s Babel-based ES→AMD transformation step with a faster Acorn + MagicString-based transpiler, and adds shared QUnit coverage to validate the new AMD wrapping and ESM semantics (imports/exports, re-exports, import.meta, circular deps).
Changes:
- Add
transpileAmd(Acorn parse + MagicString edits + identifier rewrite) and wire it intoLoader.fetchModule. - Add shared runtime-common unit tests for AMD transpilation semantics and run them from realm-server’s test suite.
- Add new JS parsing/editing dependencies (
acorn,magic-string) to runtime-common (and workspace catalog).
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Adds workspace catalog entries for Acorn-related deps. |
| pnpm-lock.yaml | Locks new catalog deps / versions (Acorn, acorn-walk, MagicString). |
| packages/runtime-common/amd-transpile.ts | New hand-rolled ES→AMD transpiler implementation. |
| packages/runtime-common/loader.ts | Switch loader to use transpileAmd and restore sourceURL for eval’d modules. |
| packages/runtime-common/package.json | Adds direct deps needed for the new transpiler. |
| packages/runtime-common/tests/amd-transpile-test.ts | Adds shared QUnit tests for the transpiler behavior. |
| packages/realm-server/tests/index.ts | Registers the new AMD transpile test module. |
| packages/realm-server/tests/amd-transpile-test.ts | Runs the shared runtime-common transpiler tests under realm-server. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Three P0 correctness bugs in the scope-aware identifier-rewrite walk
plus an unused dep — all surfaced by a fresh-eyes review after the
first round of fixes.
P0-1: shorthand property of an imported name.
`import { x } from 'foo'; const r = { x };` would naive-overwrite
the shorthand identifier, producing `{ _foo$1.x }` — a SyntaxError
that breaks any module that packs an imported binding into an object
literal. Now detect `Property.shorthand` in the value-side rewrite
and emit `x: _foo$1.x` instead.
P0-2: `for (let i = 0; ...)` shadowing an imported `i`.
The `ForStatement` / `ForInStatement` / `ForOfStatement` arm pushed
a block scope but never collected `let`/`const` bindings from the
for-head into it. Result: references to `i` inside the test/update/
body got rewritten to `_dep.i` even though `i` was a loop-local —
`'IMPORTED' < 3 → false` on the first iteration, silent wrong
answer. Now collect the for-head bindings explicitly before walking
init/test/update/body. Also fixes the `for (var k in obj)` case —
`collectFunctionScopeHoists` didn't recurse into the for-in/for-of
`left` slot, so a hoisted `var k` wasn't added to the function
scope.
P0-3: computed keys in destructured exports.
`export const { [keyName]: v } = obj` where `keyName` is imported
left the computed-key reference untouched — runtime tried to look
up an undefined property. Plain `VariableDeclaration` already calls
`walkPatternComputedKeys`; the export-namespace mirror at the
ExportNamedDeclaration walk was missing the same call. Added.
P1: `acorn-walk` was added to `runtime-common` deps and the
workspace catalog speculatively, never imported. Removed.
Plus: `stripps` → `strips` typo in the file header comment.
Adds 4 regression tests covering each P0:
- shorthand property of an imported name
- for-let loop variable shadowing an import
- for-in var hoisting through function scope
- computed key in destructured export
All 36 unit tests pass; 4-fixture validate-against-babel still
matches (52/9/17/43 deps; 68/6/1/7 export name sets).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Acorn's AST `decl.start`/`decl.end` SKIP source-level parens — for
`export default (foo);`, `decl` points at the inner `foo` (positions
inside the parens). The rewrite was using
`ms.overwrite(node.start, decl.start, 'var X = (')`
which consumed the source's leading `(` while leaving the source's
trailing `)` in place, then `ms.appendRight(decl.end, ')')` injected
another `)` BEFORE the source's `)`. Result: `var X = (foo));` — a
SyntaxError that breaks any module shipping `export default (anything)`,
`export default (function (...) {...})`, or
`export default (function(){...})()` (IIFE).
Found by an independent agent re-review; the prior tests only covered
unparenthesised default expressions.
Fix: replace just the literal `export default ` keyword (15 chars) and
append `)` before any trailing `;`. Source-level parens stay intact,
which is harmless: `var X = ((foo));` parses fine.
Adds three regression tests:
- `export default (1 + 2);`
- `export default (function () { return 9; })();` (IIFE)
- `export default (function () { return 'pf'; });` (parenthesised function expression)
39/39 unit tests pass; 4-fixture validate-against-babel still matches.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Loader runs in the browser (host), not in Node. Both test suites were wired through realm-server's qunit harness via the shared-tests pattern (`runtime-common/tests/*` + a thin `realm-server/tests/*` wrapper), but neither test exercises Node-specific code paths. Move both to `packages/host/tests/unit/` where they run in the same context the code actually executes (Ember test runner, browser). - packages/host/tests/unit/amd-transpile-test.ts (39 tests) - packages/host/tests/unit/loader-retry-test.ts (11 tests) Drops the realm-server wrappers and the runtime-common shared files. No production code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three classes of host-lint errors that didn't surface until CI ran the host's stricter eslint config (qunit-plugin rules + a tighter prettier config than runtime-common's): - `qunit/no-ok-equality` — `assert.true(x === true)` -> `assert.strictEqual(x, true)` - `qunit/no-assert-equal-boolean` — `assert.strictEqual(fn(), true)` -> `assert.true(fn())` and similarly for `false` - prettier formatting of object args spread across lines All auto-fixable via `eslint --fix`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Preview deployments |
…lation-in-loader-more-efficient
Contributor
Realm Server Test Results 1 files ± 0 1 suites ±0 17m 54s ⏱️ -10s Results for commit b7fb2ca. ± Comparison against base commit 4d80511. This pull request removes 11 tests.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b801f9b4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
P1 — assignment to imported binding silently mutated dep namespace.
`imp = 1` (or `imp++`, or `({ x } = obj)` where x is imported) was
being rewritten to `_foo$1.imp = 1` etc. — silently mutating the
dep's exports namespace. Babel emits a runtime "x is read-only" throw.
We now leave Identifier and pattern LHS positions unrewritten in
`AssignmentExpression` and `UpdateExpression` so the unbound name in
strict mode throws ReferenceError at runtime — same observable as
babel.
P2.1 — `switch` body is a single block scope.
`case 1: let y = 1; case 2: return y;` — JS scopes `y` across all
cases. The walker pushed a block scope for `SwitchStatement` (via
the default arm) but never collected `let`/`const`/`class`/`function`
decls from the case consequents into it. Added an explicit
`SwitchStatement` case that collects flat across cases before walking
the discriminant + tests + bodies.
P2.2 — class `static { ... }` block scope.
Static initializer blocks (ES2022) have their own var + lexical env.
A `let x` inside `static { ... }` must shadow an imported `x` only
within the block, not in surrounding instance methods. Added an
explicit `StaticBlock` case that pushes a function-shaped scope and
runs both function-scope hoisting + block-scope collection.
P2.3 — top-level await rejected at transpile time.
The emitted AMD factory is non-async, so source TLA would land in a
non-async wrapper at `eval(src)` time → confusing `SyntaxError`.
Added a `hasTopLevelAwait` walk and throw a clear
"top-level await is not supported by the loader" error from
`transpileAmd` instead.
Adds 5 host regression tests:
- assignment to imported binding throws
- post-increment of imported binding throws
- destructure-assign to imported binding throws
- class static block shadows imports within the block
- switch body block scope shadows imports across cases
- top-level await rejected at transpile time
- await inside an async function still works
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 5 review caught: `hasTopLevelAwait` only matched AwaitExpression nodes, but two other forms also require an async enclosing scope: - `for await (... of ...)` — ForOfStatement with `await: true`, no AwaitExpression child - `await using r = ...` — VariableDeclaration with kind `'await using'` (ES2024 explicit-resource-management) Both would have slipped past the TLA guard and produced the exact "confusing eval-time SyntaxError" the guard was supposed to prevent. Adds 4 host regression tests: - `for await (... of ...)` at top level rejected - `await using r = ...` at top level rejected - `for await (... of ...)` inside an async function still works - (existing TLA-rejection test stays green) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`collectBlockScopeDecls` and the for-loop head decl-collection only
matched `let` and `const`. ES2024 `using` and `await using` are also
block-scoped, so a same-named imported binding wasn't being shadowed.
Result: `import { r } from 'foo'; using r = ...; return r.value;`
silently rewrote `r.value` to `_foo$1.r.value` — wrong value.
Caught by an independent agent re-review; card sources today don't
use `using` (it's months-old syntax) but the gap is real once anyone
does. Two-line fix at both call sites.
Adds 2 regression tests:
- `using r = ...` shadows an imported `r`
- `for (using r of src)` head binds `r`
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Refactor:
- Extract pure helpers (`collectPatternBindings`, `isReferencePosition`)
to module level. Unifies the two slightly-different copies that lived
in `transpileAmd`'s pass 1 and the rewriter's closure.
- Convert `rewriteIdentifierReferences` (was a 580-LOC closure soup)
into an `IdentifierRewriter` class with private methods. Same shape,
same behaviour; just maintainable.
- Type the walker with acorn's `AnyNode` / `Pattern` types instead of
`any`. TypeScript now narrows on `node.type` in every switch arm and
catches `parentKey` typos statically.
- Also lands the round-7 P1 fix: `for ({ a } of items)` (destructure-
assignment in for-of/for-in head) was rewriting LHS imports the same
way `AssignmentExpression` used to. Now uses `walkPatternComputedKeys`
for ObjectPattern/ArrayPattern LHS.
Bench harness:
- Move from untracked `.bench-amd/` to tracked `scripts/bench-amd/`.
- Convert .mjs to .ts; run via ts-node (`--transpileOnly`).
- Two pnpm scripts: `bench:amd:prep` regenerates fixtures from real
card sources via transpile.ts; `bench:amd` runs the head-to-head
comparison. README documents both.
- `mise run bench:amd` wraps both.
- `fixtures/` dir stays gitignored (regenerable, large).
Bench numbers from a fresh local run (50 iter, 5 warmup):
card-api (118 KB): 13.95x over babel
spec (61 KB): 10.89x
skill-set (71 KB): 9.46x
enum (10 KB): 7.75x
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces the loader’s Babel-based ES→AMD wrapping step with a faster, hand-rolled acorn + magic-string transpiler, and adds benchmarking + unit tests to validate semantics and measure speedups.
Changes:
- Switch
Loader.fetchModuleto usetranspileAmd()and restore//# sourceURLfor better stack traces. - Add new
packages/runtime-common/amd-transpile.tsimplementation (imports/exports handling + scope-aware identifier rewriting). - Add benchmark scripts/fixtures tooling and move/add unit tests in
packages/host/tests/unit/.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Adds acorn to the workspace catalog for the new transpiler. |
| pnpm-lock.yaml | Updates lockfile for new dependencies and catalog entries. |
| packages/runtime-common/package.json | Adds acorn/magic-string deps and bench scripts. |
| packages/runtime-common/loader.ts | Replaces Babel transform with transpileAmd() and appends sourceURL on eval. |
| packages/runtime-common/amd-transpile.ts | New acorn+magic-string ES→AMD transpiler implementation. |
| packages/runtime-common/.eslintignore | Ignores generated bench fixtures directory. |
| packages/runtime-common/scripts/bench-amd/prepare-fixtures.ts | Generates realistic loader-input fixtures via the transpile pipeline. |
| packages/runtime-common/scripts/bench-amd/bench.ts | Runs wall-time benchmarks across multiple candidates/fixtures. |
| packages/runtime-common/scripts/bench-amd/README.md | Documents benchmark usage, candidates, and fixtures. |
| packages/runtime-common/scripts/bench-amd/.gitignore | Git-ignores generated fixture outputs. |
| packages/runtime-common/scripts/bench-amd/candidates/production.ts | Bench candidate that runs the production transpileAmd(). |
| packages/runtime-common/scripts/bench-amd/candidates/parse-acorn-only.ts | Bench candidate for acorn parse-only floor measurement. |
| packages/runtime-common/scripts/bench-amd/candidates/parse-esml-only.ts | Bench candidate for es-module-lexer parse-only floor measurement. |
| packages/runtime-common/scripts/bench-amd/candidates/babel-current.ts | Bench baseline candidate using Babel AMD transform. |
| packages/runtime-common/scripts/bench-amd/candidates/babel-no-sourcemap.ts | Bench baseline variant without sourcemaps. |
| packages/realm-server/tests/loader-retry-test.ts | Removes realm-server harness copy of loader retry tests. |
| packages/realm-server/tests/index.ts | Stops importing the removed realm-server loader retry test. |
| packages/host/tests/unit/loader-retry-test.ts | Rehomes loader retry tests into host unit tests and targets runtime-common loader APIs. |
| packages/host/tests/unit/amd-transpile-test.ts | Adds unit tests covering many ES module edge cases for transpileAmd(). |
| mise-tasks/bench/amd | Adds a mise task to regenerate fixtures and run the AMD transpile benchmark. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactor (no behavior change): - Break the 1100-LOC monolithic `amd-transpile.ts` into 5 cohesive files under `amd-transpile/`: - `index.ts` — `transpileAmd` public entry - `identifier-rewriter.ts` — scope-aware AST walk class - `pattern-helpers.ts` — pure AST helpers shared by main pass + walker - `emit-helpers.ts` — pure string utilities for AMD wrapper text - `top-level-await.ts` — TLA detector Copilot review fixes (PR #4573): - package.json: `es-module-lexer` and `ts-node` were in `peerDependencies` (only used by bench scripts) — moved to `devDependencies` so consumers don't get spurious peer-dep warnings. - `IdentifierRewriter`: declare `node.id.name` for `FunctionDeclaration` in its own function scope (not just `FunctionExpression`) so a recursive self-call inside `function f() { f(); }` isn't rewritten to the import accessor when `f` is also imported. - `reExportStarSnippet`: emit `configurable: true` on the getter so duplicate `export * from 'foo'` re-exports of the same key don't throw on the second `Object.defineProperty`. Matches `defineGetter` / `defineLocalGetter`. - `export default <expr>` comment: clarify that the deferred-setter pattern avoids a magic-string overlap and unifies the emit pattern, not TDZ. The expression still evaluates at its source position, so forward references to a later `const`/`class` are still a TDZ error — same as native ESM. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ktrees The amd-transpile peer→dev dep move in cf53431 didn't include the regenerated `pnpm-lock.yaml`. Also adds `.claude/worktrees/` to `.gitignore` so transient worktree gitlinks don't get caught by `git add` again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
habdelra
force-pushed
the
cs-10977-make-transpilation-in-loader-more-efficient
branch
from
b907c2b to
1551617
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR replaces the loader’s Babel-based ES→AMD wrapping with a faster hand-rolled acorn + magic-string transpiler, adds benchmarking tooling, and relocates/updates related tests.
Changes:
- Swap
Loader.fetchModulefrom BabeltransformAsync(...TransformModulesAmdPlugin...)totranspileAmd, withsourceURLrestored for stack traces. - Add new
packages/runtime-common/amd-transpile/*implementation (parse, export/import handling, identifier rewriting, TLA detection). - Add benchmarking scripts + fixtures prep and new unit tests for the AMD transpiler.
Reviewed changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Adds acorn to the workspace catalog for the new transpiler. |
| packages/runtime-common/package.json | Adds deps and bench scripts for AMD transpile benchmarking. |
| packages/runtime-common/loader.ts | Replaces Babel AMD transform with transpileAmd; adds sourceURL to eval. |
| packages/runtime-common/amd-transpile/index.ts | Implements the new ES→AMD transform and wrapper emission. |
| packages/runtime-common/amd-transpile/identifier-rewriter.ts | Adds scope-aware identifier rewrite for imports and import.meta. |
| packages/runtime-common/amd-transpile/pattern-helpers.ts | Adds AST helpers for binding collection and reference-position detection. |
| packages/runtime-common/amd-transpile/emit-helpers.ts | Adds string emit helpers for exports/re-exports/member access. |
| packages/runtime-common/amd-transpile/top-level-await.ts | Adds a top-level-await detector for clearer errors pre-eval. |
| packages/host/tests/unit/amd-transpile-test.ts | Adds unit coverage for the new transpiler behavior/semantics. |
| packages/runtime-common/tests/loader-retry-test.ts | Converts retry tests to direct QUnit tests using runtime-common loader exports. |
| packages/realm-server/tests/loader-retry-test.ts | Removes realm-server test wrapper for the shared loader retry tests. |
| packages/realm-server/tests/index.ts | Stops importing the removed loader retry test file. |
| packages/runtime-common/scripts/bench-amd/prepare-fixtures.ts | Adds fixture generation for realistic loader-input benchmark sources. |
| packages/runtime-common/scripts/bench-amd/bench.ts | Adds wall-time benchmark harness for transpilation candidates. |
| packages/runtime-common/scripts/bench-amd/README.md | Documents how to run the AMD transpile benchmark and candidates. |
| packages/runtime-common/scripts/bench-amd/.gitignore | Ignores generated benchmark fixtures. |
| packages/runtime-common/scripts/bench-amd/candidates/production.ts | Adds “production” candidate using transpileAmd. |
| packages/runtime-common/scripts/bench-amd/candidates/babel-current.ts | Adds Babel baseline benchmark candidate. |
| packages/runtime-common/scripts/bench-amd/candidates/babel-no-sourcemap.ts | Adds Babel candidate with sourcemaps disabled. |
| packages/runtime-common/scripts/bench-amd/candidates/parse-acorn-only.ts | Adds parse-only acorn floor benchmark candidate. |
| packages/runtime-common/scripts/bench-amd/candidates/parse-esml-only.ts | Adds parse-only es-module-lexer floor benchmark candidate. |
| packages/runtime-common/.eslintignore | Ignores generated bench fixtures for lint runs. |
| mise-tasks/bench/amd | Adds mise task to regenerate fixtures and run the bench. |
| .gitignore | Ignores .claude/worktrees/. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| eval(src); // + "\n//# sourceURL=" + moduleIdentifier); | ||
| // Append `sourceURL` so stack traces from inside the eval-ed AMD | ||
| // module name the original module URL instead of `<anonymous>`. | ||
| eval(src + '\n//# sourceURL=' + moduleIdentifier); |
Comment on lines
+340
to
+356
| // Replace just the `export default ` keyword (15 chars) with the | ||
| // var capture, then append `)` before any trailing `;`. We do | ||
| // NOT trim to `decl.start..decl.end` because acorn's positions | ||
| // SKIP source-level parens — for `export default (foo);`, decl | ||
| // points at `foo` (inside the parens), so consuming | ||
| // `[node.start..decl.start]` would eat the source `(` while | ||
| // leaving the source `)` untouched, producing `var X = (foo));` | ||
| // (double-paren SyntaxError). Replacing only the keyword and | ||
| // appending before `;` leaves source-level parens intact, which | ||
| // is harmless: `var X = ((foo));` parses fine. | ||
| const tempName = freshDefaultName(); | ||
| const headEnd = node.start + 'export default '.length; | ||
| ms.overwrite(node.start, headEnd, `var ${tempName} = (`); | ||
| let tail = node.end; | ||
| if (src[tail - 1] === ';') tail -= 1; | ||
| ms.appendRight(tail, ')'); | ||
| exportStatements.push(`_exports.default = ${tempName};`); |
Comment on lines
+91
to
+93
| "@glint/ember-tsc": "catalog:", | ||
| "es-module-lexer": "^1.7.0", | ||
| "ts-node": "^10.9.1" |
Comment on lines
230
to
233
| import './claim-boxel-domain-test'; | ||
| import './card-reference-resolver-test'; | ||
| import './bfm-card-references-test'; | ||
| import './loader-retry-test'; | ||
| import './package-shim-handler-test'; |
Comment on lines
+51
to
+53
| const median = sorted[Math.floor(sorted.length / 2)]; | ||
| const p95 = | ||
| sorted[Math.min(sorted.length - 1, Math.floor(sorted.length * 0.95))]; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace the loader's babel-based ES → AMD wrapping pass with a hand-rolled acorn + magic-string transpiler. ~9–13× faster than babel on real card sources, no new native deps.
Why
Indexer slowness is partly driven by the
transformAsync(... TransformModulesAmdPlugin ...)call that runs every timeLoader.fetchModulepulls a JS module. The realm-server has already lowered all the heavy syntax (TS, decorators, glimmer templates, scoped CSS) by the time the loader sees the source — so the loader's babel pass is doing nothing but ES → AMD wrapping, and paying the full price of babel's plugin pipeline + AST visitors + code generation.What changed
packages/runtime-common/amd-transpile.ts— acorn parse + magic-string string-edits, two-pass: first collects everyImportDeclarationbinding into a name → dep-arg-access map; second emits thedefine(moduleId, deps, factory)wrapper, transforms exports, and runs a scope-aware identifier-rewrite walk that replaces every non-shadowed source-code reference to an imported name with_dep.name. Lookup happens at use time, so circular deps and forward references work.packages/runtime-common/loader.ts—Loader.fetchModulenow callstranspileAmdinstead oftransformAsync+TransformModulesAmdPlugin.eval(src)regains a//# sourceURL=<moduleId>hint so stack traces name the original module.runtime-common:acorn,magic-string(both already transitive in the workspace; added to the catalog).packages/host/tests/unit/(the loader runs in the browser, not in Node):amd-transpile-test.ts— 39 tests covering imports, exports, re-exports,export *,import.meta, live-binding, circular-dep evaluation order, TDZ-safe default exports, destructured exports, parenthesised default expressions, IIFE defaults,__default$Ncollision avoidance, identifier shadowing, for-loop scope tracking, shorthand-property of imports, computed-key destructuring.loader-retry-test.ts— 11 tests forfetchWithTransientRetry(was previously a runtime-common shared file wired through realm-server's qunit harness; moved alongside).Benchmarks
Tested against post-realm-server-transpile output of real card modules — exactly what the loader's transpile pass sees today. 50 iterations + 5 warmup, median wall time. Latest run on the current commit (paren-fix + scope-aware rewrite + getter-based exports + TDZ-safe defaults):
enum.jsskill-set.jsspec.jscard-api.jsParse-only floors for context (we already pay the parse cost):
enum.jscard-api.jsWe use acorn because the scope-aware identifier-rewrite walk needs a full AST; es-module-lexer only exposes import/export byte ranges. SWC native (also benchmarked earlier) lands at 6–12× over babel — comparable to what we get here, but adds a ~30 MB native binary across platforms.
Disabling sourcemaps on the babel pipeline gives only 1.0–1.2× — sourcemap encoding is not the bottleneck.
Live-binding semantics
Preserved across module boundaries via three mechanisms:
_dep.name. Lookup happens at use-time, so by the time card code callsfield(...)from inside a method, the dep has finished evaluating and the export is populated. This is what makes circular deps work (e.g.card-api.gts↔contains-many-component.gts).export { x } from 'foo',export *,export { localImported }) — getter that reads through the dep arg every time.export let X/export var X) — getter that reads the local binding at access time, so mutations made by exported functions (export function inc() { X++ }) propagate to importers.export default <expression>strips just theexport defaultkeyword (15 chars) and appends)before any trailing;, leaving any source-level parens intact (soexport default (foo);becomesvar __default$0 = ((foo));— valid, parens harmless). The_exports.defaultsetter is deferred to the end of the body — TDZ-safe and avoids magic-string overlap with the identifier-rewrite walk. The__default$Nname is collision-checked against every top-level declared name in the source.Validated
export letmutated by an exported function,export default <imported_name>,export default (paren-expr)and IIFE defaults, destructuredexport const { a, b } = obj, computed-key destructuring,__default$Ncollision avoidance, identifier shadowing, for-let loop variable shadowing imports, for-invarhoisting, shorthand-property of imported names.card-api.gts,enum.gts,skill-set.gts,spec.gts.Other candidates considered, rejected
node_modulesfor marginal benefit.Test plan