Adopt Explicit Resource Management (using/Symbol.dispose) - #875
Merged
Conversation
Raise TypeScript target to ESNext (root tsconfig) and align every build path (tsc, rollup+esbuild, vite) so `using`/`await using` compile to native V8 syntax instead of downlevel helpers. - Drop `lib` overrides in runtime/cli/file-io tsconfigs so they inherit ESNext (Disposable/DisposableStack types were missing under ES2020/ES2024) - Point blocks/custom-element tsconfig.rollup.json at the root tsconfig instead of @d-zero/tsconfig directly, so the target bump applies - Pin `target: 'esnext'` on rollup-plugin-esbuild calls (blocks, custom-element, legacy) since it otherwise defaults to es2017 - Raise runtime's vite build.target from es2020 to esnext - Add canary specs verifying `using`/DisposableStack/AsyncDisposableStack run natively across the default/client/core-browser vitest projects
Both releases add Explicit Resource Management support (0.23.0 adds mkdtempDisposable/disposableListener); blocks already depended on shared and cli on roar. Add @d-zero/shared as a devDependency to cli/local/mcp-server/file-io so their tests can adopt mkdtempDisposable in a later commit.
…bserver/engine Give the engine's core resources a `using`-compatible teardown path and fix leaks that public destroy()/off()/cleanUp() never covered. - HealthMonitor, CommandBus, ComponentObserver now implement Disposable; their old destroy()/off()/stop() methods are deprecated thin wrappers around [Symbol.dispose], with the real teardown moved into private #destroy()/#off()/#stop() methods - CommandBus#listen() and ComponentObserver#on() now return a detach function that is also Disposable, so callers can hold it in a `using` declaration instead of invoking it manually - Add beginProcessing(engine) in a new processing-scope.ts: replaces the `engine.isProcessed = true; ...; engine.isProcessed = false` pattern, which left the flag stuck on true (freezing all engine commands) whenever the code in between threw. Applied to InsertionPoint#insert, which had exactly this leak on its "not added to the DOM tree" path - BurgerEditorView now extends Disposable (destroy() kept as a deprecated alias); BurgerEditorEngine wires healthMonitor/commandBus/ componentObserver/view and every stylesheet blob URL into a DisposableStack, so engine[Symbol.dispose]() (and the now-deprecated cleanUp()) also stops the componentObserver's window listeners and revokes blob URLs that were never released before - Convert health-monitor.spec.ts's manual start()/stop() pairs to `using` declarations
…e alias Align BurgerEditorView with the same reversal already applied to HealthMonitor/CommandBus/ComponentObserver: the real teardown lives in [Symbol.dispose] (backed by a closure in createDefaultView, since this is a plain object rather than a class), and destroy() is now a thin @deprecated wrapper that forwards to it.
…t leak - register-engine-commands.ts: wrap the moveBlock/removeBlock handlers' await in `using _processing = beginProcessing(engine)` blocks, so `engine.isProcessed` resets to false even if replaceElement() rejects (previously it stayed stuck on true, freezing every other engine command). The `using` block is scoped to end before `engine.save()`, preserving the existing false-then-save() order - create-react-view.tsx: same destroy()-deprecated/[Symbol.dispose] reversal as core's view implementations - mount.tsx: reactMount() now returns a Disposable handle; cleanUp() is a deprecated alias - index.tsx: register the dialog host's React root + element with engine.own(...), so engine[Symbol.dispose]() actually unmounts it — previously the mount handle was discarded and the host div leaked on every engine teardown
Both bge-wysiwyg and bge-wysiwyg-editor never tore anything down: the tiptap Editor's ProseMirror view/plugins/listeners, shadow DOM event listeners and a textarea value descriptor override all leaked on every unmount (dialog close, block removal, page navigation). Strategy: lazy dispose + explicit [Symbol.dispose]. disconnectedCallback captures the current DisposableStack and defers its disposal to a microtask, checking `!this.isConnected` first so a same-tick DOM move doesn't tear anything down. connectedCallback synchronously disposes any live stack before rebuilding, since the WYSIWYG iframe's browsing context is destroyed the instant it's detached — state can't survive a move regardless, so full reinitialization is unavoidable. Disposal writes the current value back to plain light DOM first, so content (not undo history) survives across the reinit. connectedMoveCallback is defined as a no-op so a future `Node.moveBefore()`-based move (Chrome 133+) preserves the iframe and skips teardown entirely. - bge-wysiwyg-element: DisposableStack tears down the Editor (editor.destroy()), the AbortController backing every DOM listener, and the textarea's overridden `value` property descriptor (now restored via Reflect.deleteProperty, which required adding `configurable: true` to the original defineProperty) - text-only-mode.ts: TextOnlyModeController implements Disposable; deactivate() still just empties the container for mode switching, [Symbol.dispose] additionally removes the container node itself - bge-wysiwyg-editor-element: same DisposableStack treatment for its own listeners, plus writes the inner element's value back into plain (non-toolbar) innerHTML and disposes the child element explicitly. connectedCallback now no-ops while its stack is still live (covers same-tick moves) instead of the old one-shot #initialized flag - item-editor-host.tsx: guard the deferred getContentStylesheet().then() against a since-disposed element, which would otherwise throw inside an unhandled promise rejection if the dialog closes before it settles
item-editor-host.tsx queries the mounted <bge-wysiwyg-editor> and calls setStyle() once engine.getContentStylesheet() resolves. If the dialog closes before that promise settles, the custom element is now disposed and setStyle() throws inside an unhandled rejection. Track a cancelled flag from the effect cleanup to skip the call in that case.
bin.ts's stdout-redirect try/finally becomes a one-line `using` declaration. Behavior is unchanged — dotenv's stdout banner still gets redirected to stderr only for the duration of loadContext().
scanHtmlFiles/scanHtmlFilesWithMultipleQueries created a new JSDOM per file inside their scan loop and never closed it, accumulating jsdom windows (and their timers) for the process lifetime on large document roots. Wrap each parse in openDom(), a using-compatible helper that closes the window when the scope exits.
Mirrors the destroy()/[Symbol.dispose] reversal applied elsewhere: unmount() is now a deprecated wrapper forwarding to [Symbol.dispose], which does the actual root.unmount(). No behavior change for existing callers.
- virtual-path-resolver.spec.ts / edit-content.spec.ts: replace manual fs.mkdtemp()+fs.rm() pairs with @d-zero/shared's mkdtempDisposable, disposed explicitly in afterEach/afterAll (hooks can't use `using` directly since the handle must outlive the setup callback) - edit-content.spec.ts also drops the beforeEach/afterAll asymmetry — mkdtempDisposable's directory is created once in beforeAll, matching what the old beforeEach's idempotent recreate already did in practice - virtual-path-resolver.spec.ts: replace the vi.spyOn()+try/finally around a mocked fs.readFile with disposableSpy(), a new __tests__/ helper wrapping vi.spyOn() as Disposable - Add vitest as a devDependency (needed directly now that a non-.spec file imports it) and exclude src/__tests__/** from tsconfig.build.json so the helper doesn't ship in dist
- spec-input.spec.ts: replace the withMockStdin(isTTY, payload, run) HOF with mockStdin(isTTY, payload), a Disposable held in a `using` declaration — flattens every call site (no more nested callback) and restores process.stdin the same way a `using` restores any resource - handlers.spec.ts: replace the manual fs.mkdtemp()+fs.rm() pair with mkdtempDisposable; replace the chmod 0o555→finally→0o755 dance around the EACCES rename test with chmodScoped(), an AsyncDisposable - Add src/__tests__/disposables.ts with these three helpers and exclude it from tsconfig.build.json
Neither client nor server was ever closed — every run leaked an InMemoryTransport pair. Replace the ad-hoc tmpRoot/originalCwd cleanup with an AsyncDisposableStack: registration follows creation order, so disposeAsync() tears down client → server → cwd restore → tmp dir removal (in that order), fixing the leak alongside the existing cleanup.
makeTmpDocumentRoot() now returns the mkdtempDisposable handle alongside documentRoot/assetsRoot; all 9 describe blocks' afterEach hooks dispose it instead of manually computing path.dirname(documentRoot) and fs.rm()'ing it.
- server.spec.ts / search.spec.ts: replace manual fs.mkdtemp()+fs.rm() pairs with mkdtempDisposable - load-resolver-state-or-exit.spec.ts: same for its documentRoot, plus disposableSpy() for the console.error/process.exit spies (previously restored manually in afterEach) - Add src/__tests__/disposables.ts (mirrors file-io's helper) and add vitest as a devDependency, needed now that a non-.spec file imports it
captureStartupStderr()'s 15s hard-cap setTimeout was never cleared when the child matched the expected stderr line early (the common case) or when spawn itself errored — leaving a dangling timer that fires later, kills an already-dead child, and keeps the event loop (and vitest's teardown) waiting on it for up to 15s per test.
Purely mechanical reordering (lint-staged only checks staged diffs, so this surfaced on the first full `yarn lint`). No behavior change.
…inding
Code review findings from the ERM migration:
- engine.ts: BurgerEditorEngine.new() split into new() (owns error
handling) + a private #finishConstruction(). If construction throws
partway through (e.g. a stylesheet fetch in Promise.all rejects), the
half-built engine's already-deferred blob URLs and used view had no
way to be disposed — it never left this function, so callers had no
[Symbol.dispose] to call. new() now disposes the partial engine and
rethrows on any failure
- default-view.ts / types.ts: createDefaultView()'s destroy() called
`this[Symbol.dispose]()`, which throws if a caller pulls `destroy`
off the returned object before calling it (this-binding is lost).
destroy and [Symbol.dispose] now point at the same closure instead of
routing through `this`. Updated the BurgerEditorView JSDoc example to
show the safe pattern
- command-bus.ts / component-observer.ts: extract the repeated
`Object.assign(fn, { [Symbol.dispose]: fn })` pattern into
utils/as-disposable-fn.ts
… hiding it If <bge-wysiwyg> disposes itself (via its own disconnectedCallback race) before the outer <bge-wysiwyg-editor>'s deferred value write-back runs, reading wysiwygElement.value throws and was silently swallowed. Log a dev-only console.warn so the skipped write-back is observable instead of invisible.
…alias
`cleanUp() { this[Symbol.dispose](); }` throws if a caller pulls
`cleanUp` off the returned handle before calling it — `this` is lost.
cleanUp and [Symbol.dispose] now point at the same closure instead of
routing through `this`. Same fix applied to createReactView()'s
destroy() in the previous core commit; add a regression test here
covering the destructured-call case.
…lias Same fix as the client/core commits: unmount and [Symbol.dispose] now point at the same closure instead of unmount() routing through `this[Symbol.dispose]()`.
as-disposable-fn.ts, and the listen()/on() return values that use it, had no test exercising the Disposable branch — every existing spec only called the returned detach function directly. Add a dedicated spec plus a `using`-based regression test in each of command-bus.spec.ts and component-observer.spec.ts.
The cancelled flag added around getContentStylesheet().then(setStyle) had no test — nothing exercised the unmount-before-resolve path it exists to guard. Add a spec using a stub custom element registered under the bge-wysiwyg-editor tag name (avoids pulling in the real tiptap-backed element, which needs a real browser) to verify setStyle is skipped after unmount and still called when not unmounted.
CI's client:build (tsc -p tsconfig.check.json) caught two issues local `yarn build` runs before this test file existed had missed: - Importing ItemEditorHost via the '@burger-editor/client/ui' package alias from inside the client package itself doesn't resolve under tsc's type-checking context (only vitest's alias resolves it) — use the relative import instead - wysiwygStubSeed was missing the required `style` field on ItemSeed
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
1 participant
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.
概要
Explicit Resource Management（
using/await using/Symbol.dispose/DisposableStack）を導入する。動作環境を Node >=24.11 / 最新 Chrome に限定する前提で、tsconfig のtargetをESNextに上げ、V8 ネイティブのusingで emit する（downlevel ヘルパーは使わない）。調査の結果、リポジトリ内の
try/finallyは5箇所のみだった。真の価値は以下の3点:engine.isProcessedが例外発生時に立ったままになり、UI が「処理中」表示で固まって以降の全エンジンコマンドが無効化されるバグを修正componentObserver.off()未呼び出し（window リスナーリーク）、blob URL の revoke 漏れ、tiptapEditor.destroy()未呼び出し（最大級のリーク）、jsdom のループ内生成でclose()漏れ、BurgerEditorEngine.new()の構築途中失敗によるリークmkdtemp/fs.rmの対応漏れリスク、spawn の kill 漏れ・タイマーリーク、vi.spyOnのmockRestore()忘れを@d-zero/sharedの ERM ヘルパー（mkdtempDisposable）や自作ヘルパーで解消主な変更
target: "ESNext"に。rollup/vite/tsconfig.rollup.json も ESNext ネイティブ emit に揃える。@d-zero/shared0.23.0 /@d-zero/roar2.2.1 に更新HealthMonitor/CommandBus/ComponentObserver/BurgerEditorEngineにSymbol.disposeを実装。旧来のdestroy()/stop()/off()/cleanUp()は@deprecatedの薄いラッパーに（内部の実処理は private メソッドへ移動）。beginProcessing(engine)を新設し、isProcessedの一時変更をusingで安全に。BurgerEditorViewはDisposableを継承（destroy()は互換のため維持）register-engine-commands.tsの move/remove-block ハンドラをbeginProcessingで書き換え、dialogHostの React root を engine のDisposableStackに登録してリークを修正bge-wysiwyg-element/bge-wysiwyg-editor-elementにdisconnectedCallback（マイクロタスク遅延 dispose）+DisposableStackを導入し、tiptapEditorを確実に破棄する。同一タスク内の DOM 移動では即時 dispose せず、connectedMoveCallbackを no-op にして将来のNode.moveBefore()対応にも備えるsilenceStdout()/openDom()（jsdom ラッパー）/FrontMatterEditorHandleの dispose 対応file-io/cli/local/mcp-serverの一時ディレクトリ・スパイ・stdin モックを ERM ヘルパー化。mcp-serverのv4.spec.tsではClient/Transportの close 漏れも修正BurgerEditorEngine.new()の構築途中失敗時のリソースリーク修正、this[Symbol.dispose]()経由のthis束縛問題（分割代入で呼ぶとTypeError）を解消、Object.assign(fn, {[Symbol.dispose]: fn})の重複をasDisposableFnに共通化、親子カスタム要素の dispose レース時に無音で失敗していた箇所を dev 警告化スコープ外（issue化済み）
今回の調査で見つかった関連リークだが、本 PR のスコープ外として issue 化した:
file-uploader.tsxの blob URL 未 revokelocalの Hono server ハンドル未保持mcp-serverの stdio シングルトンに close 経路がないcleanUp()の乱暴な DOM 全削除GoogleMapsEditorのdragTimer未クリアblock-menu.tsx等のuseEffectをDisposableStack化する余地inspectorパッケージのテストがvitest.config.tsに接続されていない（既存の構造的ギャップ）検証
yarn build（NX_WORKSPACE_ROOT_PATH指定）→ 成功、dist に downlevel ヘルパー（__addDisposableResource等）の混入なしyarn lint→ 成功yarn test（Docker 経由、VR 含む）→ 1054 件成功 / 1件スキップ（root ユーザー環境での既知の意図的スキップ）、VR 5ファイル31件成功（見た目の差分なし）/code-review mediumと QA エンジニアレビューを実施し、指摘事項を全て対応（テスト追加含む）