test: compile() inside a scheduler-driven system + the call-once pattern#89
Conversation
compile() (like bindColumns) is a call-ONCE API, but a SystemDef has only a per-frame `run` and no init hook. The idiomatic pattern is to lazily build the runner on the first frame and cache it in the system's closure. This locks that in end-to-end and documents it. - new packages/scheduler/test/compile-in-system.integration.test.ts: a lazily-cached compiled runner integrates correctly across frames under scheduler.update() (built exactly once), matches an equivalent .each system byte-for-byte, and a .changed(Position) consumer sees the compiled writes every frame (reactivity holds through the scheduler). - performance.md: "Inside a system" subsection showing the `run ??= query(...).compile(...)` lazy-cache pattern + the note that compile() is a main-thread run-body tool (worker-eligible systems run their separately-authored kernel on workers).
Greptile SummaryThis PR adds an integration test and documentation for the
Confidence Score: 4/5Safe to merge for single-world usage; the module-level runner cache in the docs example warrants a note or pattern change before it teaches users a multi-world footgun. The test logic is correct and well-isolated. The doc example's module-level The 'Inside a system' example in website/guide/performance.md deserves a second look for the module-scope runner cache. Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
website/guide/performance.md:209-238
**Module-level runner silently shared across worlds**
The example declares `let move` outside `defineSystem`, so a single `Movement` constant reuses the first world's compiled runner for every subsequent world that runs it. The runner is bound to the first world's typed-array columns; applying it to a different world's entities reads and writes the wrong memory. The idiomatic fix is to move the lazy initializer inside a factory function so each invocation captures its own slot.
### Issue 2 of 2
packages/scheduler/test/compile-in-system.integration.test.ts:13-17
**Local `CompileQuery` type diverges from the public `Query` interface**
The public `Query<Terms>` interface in `@ecsia/schema` already exposes `compile<Ctx>(body)`, typed against the inferred `QueryElement<Terms>`. The local `CompileQuery` alias hard-codes the element shape, so a schema change would allow the test to compile and pass while the body silently omits the new field. Using the public `Query` type from `@ecsia/schema` would catch such drift at compile time.
Reviews (1): Last reviewed commit: "test: compile() inside a scheduler-drive..." | Re-trigger Greptile |
| ### Inside a system | ||
|
|
||
| A `defineSystem` has only a per-frame `run` — no separate setup step — so build the runner on the first | ||
| frame and cache it in the system's closure (the same pattern applies to `bindColumns`). The query you | ||
| need is the one `run` receives, so the lazy build is the natural place for it: | ||
|
|
||
| ```ts no-check | ||
| let move: ((ctx: { dt: number }) => void) | null = null | ||
|
|
||
| const Movement = defineSystem({ | ||
| name: 'Movement', | ||
| read: [Velocity], | ||
| write: [Position], | ||
| run({ query, dt }) { | ||
| move ??= query(read(Velocity), write(Position)).compile<{ dt: number }>((e, ctx) => { | ||
| e.position.x += e.velocity.dx * ctx.dt | ||
| e.position.y += e.velocity.dy * ctx.dt | ||
| }) | ||
| move({ dt }) | ||
| }, | ||
| }) | ||
| ``` | ||
|
|
||
| The runner is built once, on the first frame, then reused — and because `compile` preserves the write | ||
| log, a `.changed()`/observer system later in the schedule still sees the writes. (A worker-eligible | ||
| system runs its separately-authored kernel on worker threads; `compile` is a main-thread `run`-body | ||
| tool, exactly like `each` and `bindColumns`.) | ||
|
|
||
| ## Reproduce | ||
|
|
There was a problem hiding this comment.
Module-level runner silently shared across worlds
The example declares let move outside defineSystem, so a single Movement constant reuses the first world's compiled runner for every subsequent world that runs it. The runner is bound to the first world's typed-array columns; applying it to a different world's entities reads and writes the wrong memory. The idiomatic fix is to move the lazy initializer inside a factory function so each invocation captures its own slot.
Prompt To Fix With AI
This is a comment left during a code review.
Path: website/guide/performance.md
Line: 209-238
Comment:
**Module-level runner silently shared across worlds**
The example declares `let move` outside `defineSystem`, so a single `Movement` constant reuses the first world's compiled runner for every subsequent world that runs it. The runner is bound to the first world's typed-array columns; applying it to a different world's entities reads and writes the wrong memory. The idiomatic fix is to move the lazy initializer inside a factory function so each invocation captures its own slot.
How can I resolve this? If you propose a fix, please make it concise.| type CompileQuery = { | ||
| compile<Ctx>(body: (e: { position: { x: number; y: number }; velocity: { dx: number; dy: number } }, ctx: Ctx) => void): ( | ||
| ctx: Ctx, | ||
| ) => void | ||
| } |
There was a problem hiding this comment.
Local
CompileQuery type diverges from the public Query interface
The public Query<Terms> interface in @ecsia/schema already exposes compile<Ctx>(body), typed against the inferred QueryElement<Terms>. The local CompileQuery alias hard-codes the element shape, so a schema change would allow the test to compile and pass while the body silently omits the new field. Using the public Query type from @ecsia/schema would catch such drift at compile time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/scheduler/test/compile-in-system.integration.test.ts
Line: 13-17
Comment:
**Local `CompileQuery` type diverges from the public `Query` interface**
The public `Query<Terms>` interface in `@ecsia/schema` already exposes `compile<Ctx>(body)`, typed against the inferred `QueryElement<Terms>`. The local `CompileQuery` alias hard-codes the element shape, so a schema change would allow the test to compile and pass while the body silently omits the new field. Using the public `Query` type from `@ecsia/schema` would catch such drift at compile time.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
What
compile()(likebindColumns) is a call-once API — it codegens per-archetype runners — but aSystemDefhas only a per-framerunand no init hook. The idiomatic pattern is to lazily build the runner on the first frame and cache it in the system's closure. This PR locks that in with an integration test and documents it.Why
compile()was property-tested at the bare-query level (#86) but never exercised in its primary execution context — a system undercreateScheduler. This closes that gap and answers the natural "how do I use a call-once API in a per-framerun?" question. (Worker-eligible systems run a separately-authored kernel on workers;compileis a main-threadrun-body tool, exactly likeeach/bindColumns— confirmed while investigating.)Changes
packages/scheduler/test/compile-in-system.integration.test.ts— a lazily-cached compiled runner (a) integrates correctly across frames underscheduler.update(), built exactly once; (b) matches an equivalent.eachsystem byte-for-byte; (c) a.changed(Position)consumer sees the compiled writes every frame (reactivity holds through the scheduler).run ??= query(...).compile(...)pattern.Test plan
pnpm typecheck:tests,pnpm docs:check