Skip to content

Commit 39d0031

Browse files
authored
Merge pull request #337 from knockout/fix/jsx-clean-teardown-race
Fix intermittent happy-dom teardown race in JSX clean queue
2 parents 802ac55 + 22dad97 commit 39d0031

8 files changed

Lines changed: 266 additions & 8 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
"@tko/utils.jsx": patch
3+
---
4+
5+
Add `options.jsxCleanBatchSize` (default `1000`) controlling JSX node cleanup
6+
batching. Setting it to `0` runs cleanup synchronously on detach. Registered
7+
via `defineOption` β€” the hardcoded `MAX_CLEAN_AT_ONCE` constant is gone, but
8+
the new default matches it so production behavior is unchanged.
9+
10+
Fixes a `ReferenceError: Element is not defined` in the `cli-happy-dom` test
11+
project where the 25ms batch timer could fire after happy-dom had torn down
12+
DOM globals.

β€ŽAGENTS.mdβ€Ž

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,13 @@ packages/example/
105105
Inter-package dependencies use `@tko/package-name` and are resolved via
106106
npm workspaces.
107107

108+
### Configurable runtime options (`ko.options.*`)
109+
110+
Register package-owned options via `defineOption` from `@tko/utils`, not as
111+
fields on the core `Options` class. See
112+
[`tko.io/public/agents/options.md`](tko.io/public/agents/options.md) for the
113+
pattern and canonical example.
114+
108115
## CI/CD
109116

110117
GitHub Actions workflows (`.github/workflows/`):

β€Žbuilds/knockout/helpers/vitest-setup.jsβ€Ž

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import * as chai from 'chai'
22
import sinon from 'sinon'
3+
import { options } from '@tko/utils'
34
import { isHappyDom } from '../../../packages/utils/helpers/test-env.ts'
45

56
// Set globals that builds/knockout specs and mocha-test-helpers.js expect
@@ -20,3 +21,11 @@ import '../dist/browser.min.js'
2021
// Now import the helper β€” it needs chai, expect, ko, and beforeEach/afterEach as globals.
2122
// beforeEach/afterEach come from vitest globals (globals: true in config).
2223
import './mocha-test-helpers.js'
24+
25+
// Run JSX cleanup synchronously in tests. In happy-dom the 25ms batch
26+
// timer can fire after the DOM globals are torn down, throwing from
27+
// `cleanNode`. browser.min.js bundles its own Options instance, so mirror.
28+
beforeAll(() => {
29+
options.jsxCleanBatchSize = 0
30+
globalThis.ko.options.jsxCleanBatchSize = 0
31+
})
Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
import { assert } from 'chai'
2+
import sinon from 'sinon'
3+
4+
import { addDisposeCallback, options } from '@tko/utils'
5+
6+
import { queueCleanNode } from '../src/jsxClean'
7+
8+
describe('jsxClean queue', function () {
9+
let cleaned: Node[]
10+
let clock: sinon.SinonFakeTimers
11+
let originalBatchSize: number
12+
13+
function makeNode(): HTMLElement {
14+
const node = document.createElement('div')
15+
addDisposeCallback(node, () => cleaned.push(node))
16+
return node
17+
}
18+
19+
beforeEach(function () {
20+
cleaned = []
21+
originalBatchSize = options.jsxCleanBatchSize
22+
clock = sinon.useFakeTimers()
23+
})
24+
25+
afterEach(function () {
26+
clock.restore()
27+
options.jsxCleanBatchSize = originalBatchSize
28+
})
29+
30+
describe('jsxCleanBatchSize = 0 (synchronous)', function () {
31+
beforeEach(function () {
32+
options.jsxCleanBatchSize = 0
33+
})
34+
35+
it('runs cleanup synchronously on queueCleanNode', function () {
36+
const node = makeNode()
37+
queueCleanNode(node)
38+
assert.deepEqual(cleaned, [node])
39+
})
40+
41+
it('drains all queued nodes synchronously in a single call', function () {
42+
const nodes = [makeNode(), makeNode(), makeNode()]
43+
for (const n of nodes) queueCleanNode(n)
44+
assert.deepEqual(cleaned, nodes)
45+
})
46+
47+
it('does not schedule a timer', function () {
48+
queueCleanNode(makeNode())
49+
assert.equal(clock.countTimers(), 0)
50+
})
51+
})
52+
53+
describe('jsxCleanBatchSize > 0 (batched, default 1000)', function () {
54+
beforeEach(function () {
55+
options.jsxCleanBatchSize = 1000
56+
})
57+
58+
it('defers cleanup until the 25ms timer fires', function () {
59+
const node = makeNode()
60+
queueCleanNode(node)
61+
assert.lengthOf(cleaned, 0, 'not cleaned before timer fires')
62+
clock.tick(25)
63+
assert.deepEqual(cleaned, [node])
64+
})
65+
66+
it('does not re-trigger the timer while one is already pending', function () {
67+
const a = makeNode()
68+
const b = makeNode()
69+
queueCleanNode(a)
70+
queueCleanNode(b)
71+
clock.tick(24)
72+
assert.lengthOf(cleaned, 0)
73+
clock.tick(1)
74+
assert.deepEqual(cleaned, [a, b])
75+
})
76+
77+
it('processes at most batchSize nodes per 25ms tick', function () {
78+
options.jsxCleanBatchSize = 3
79+
const nodes = [makeNode(), makeNode(), makeNode(), makeNode(), makeNode()]
80+
for (const n of nodes) queueCleanNode(n)
81+
82+
clock.tick(25)
83+
assert.lengthOf(cleaned, 3, 'first tick processes one batch')
84+
85+
clock.tick(25)
86+
assert.lengthOf(cleaned, 5, 'second tick drains the remainder')
87+
})
88+
89+
it('re-schedules itself when the queue is non-empty after a batch', function () {
90+
options.jsxCleanBatchSize = 2
91+
const nodes = Array.from({ length: 5 }, makeNode)
92+
for (const n of nodes) queueCleanNode(n)
93+
94+
clock.tick(25)
95+
assert.lengthOf(cleaned, 2)
96+
clock.tick(25)
97+
assert.lengthOf(cleaned, 4)
98+
clock.tick(25)
99+
assert.lengthOf(cleaned, 5)
100+
})
101+
102+
it('drains synchronously if batchSize is flipped to 0 while a timer is pending', function () {
103+
const nodes = [makeNode(), makeNode(), makeNode()]
104+
for (const n of nodes) queueCleanNode(n)
105+
assert.equal(clock.countTimers(), 1)
106+
107+
options.jsxCleanBatchSize = 0
108+
clock.tick(25)
109+
110+
assert.deepEqual(cleaned, nodes)
111+
assert.equal(clock.countTimers(), 0, 'no re-armed timer')
112+
})
113+
114+
it('drains synchronously if batchSize becomes non-finite while a timer is pending', function () {
115+
const nodes = [makeNode(), makeNode()]
116+
for (const n of nodes) queueCleanNode(n)
117+
118+
options.jsxCleanBatchSize = Number.NaN as unknown as number
119+
clock.tick(25)
120+
121+
assert.deepEqual(cleaned, nodes)
122+
assert.equal(clock.countTimers(), 0)
123+
})
124+
})
125+
})
Lines changed: 44 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,62 @@
1-
import { cleanNode } from '@tko/utils'
1+
import { cleanNode, defineOption, options } from '@tko/utils'
22

33
const DELAY_MS = 25
4-
const MAX_CLEAN_AT_ONCE = 1000
54
const cleanNodeQueue = new Array()
65
let cleanNodeTimeoutID: ReturnType<typeof setTimeout> | null = null
76

7+
// Extend the Options type so ko.options.jsxCleanBatchSize is strongly typed.
8+
declare module '@tko/utils' {
9+
interface Options {
10+
jsxCleanBatchSize: number
11+
}
12+
}
13+
14+
// `0` runs cleanup synchronously on detach. Test environments that tear
15+
// down DOM globals between files use that to avoid a pending 25ms timer
16+
// firing against a dead global.
17+
defineOption('jsxCleanBatchSize', { default: 1000 })
18+
819
export function queueCleanNode(node) {
920
cleanNodeQueue.push(node)
10-
triggerCleanTimeout()
21+
if (options.jsxCleanBatchSize === 0) {
22+
flushAll()
23+
} else {
24+
scheduleBatch()
25+
}
1126
}
1227

13-
function triggerCleanTimeout() {
28+
function scheduleBatch() {
1429
if (!cleanNodeTimeoutID && cleanNodeQueue.length) {
15-
cleanNodeTimeoutID = setTimeout(flushCleanQueue, DELAY_MS)
30+
cleanNodeTimeoutID = setTimeout(flushBatch, DELAY_MS)
1631
}
1732
}
1833

19-
function flushCleanQueue() {
34+
function flushBatch() {
2035
cleanNodeTimeoutID = null
21-
const nodes = cleanNodeQueue.splice(0, MAX_CLEAN_AT_ONCE)
36+
// If the option was flipped to a non-positive / non-finite value while the
37+
// timer was pending, fall through to synchronous drain β€” otherwise
38+
// splice(0, <=0|NaN) removes nothing and scheduleBatch re-arms forever.
39+
const batchSize = Math.trunc(options.jsxCleanBatchSize)
40+
if (!Number.isFinite(batchSize) || batchSize <= 0) {
41+
flushAll()
42+
return
43+
}
44+
const nodes = cleanNodeQueue.splice(0, batchSize)
2245
for (const node of nodes) {
2346
cleanNode(node)
2447
}
25-
triggerCleanTimeout()
48+
scheduleBatch()
49+
}
50+
51+
function flushAll() {
52+
if (cleanNodeTimeoutID !== null) {
53+
clearTimeout(cleanNodeTimeoutID)
54+
cleanNodeTimeoutID = null
55+
}
56+
// Outer `while` is for re-enqueues triggered by cleanNode side effects.
57+
while (cleanNodeQueue.length) {
58+
for (const node of cleanNodeQueue.splice(0)) {
59+
cleanNode(node)
60+
}
61+
}
2662
}

β€Žtko.io/public/agents/guide.mdβ€Ž

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ Test-backed behavior summaries live under `/agents/verified-behaviors/`. Treat t
44

55
For preferred state/binding/DOM architecture in examples and prototypes, read `/agents/contract.md`.
66

7+
For how to add or configure runtime options (`ko.options.*`), read `/agents/options.md`.
8+
79
## Setup
810

911
```html
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
# `ko.options.*` β€” Configurable Runtime Options
2+
3+
`ko.options` is TKO's runtime configuration object. It is a singleton `Options`
4+
instance defined in `packages/utils/src/options.ts`. This page documents how
5+
to add new options and which of the two available mechanisms to reach for.
6+
7+
## Two mechanisms β€” when to use which
8+
9+
### `defineOption` (default β€” almost always use this)
10+
11+
For options that belong to a specific package (batch sizes, feature toggles,
12+
plugin behavior), register via `defineOption` inside the owning package.
13+
14+
```ts
15+
// packages/utils.jsx/src/jsxClean.ts
16+
import { defineOption, options } from '@tko/utils'
17+
18+
// Extend the Options type so ko.options.<name> is strongly typed.
19+
declare module '@tko/utils' {
20+
interface Options {
21+
jsxCleanBatchSize: number
22+
}
23+
}
24+
25+
// Register at module load, with an optional side-effect setter.
26+
defineOption('jsxCleanBatchSize', { default: 1000 })
27+
28+
// Read wherever the option applies.
29+
if (options.jsxCleanBatchSize === 0) { /* sync path */ }
30+
```
31+
32+
Rules:
33+
34+
- The `declare module '@tko/utils' { interface Options { ... } }` augmentation
35+
lives in the same file (or at least the same package) that defines the option.
36+
- `defineOption` registers at module-side-effect time; the option is available
37+
as soon as the owning package is imported.
38+
- An optional `set(value)` runs side effects at configuration time (not
39+
retroactively on already-parsed bindings) β€” use for options that swap
40+
implementations or rebuild internal state.
41+
42+
**Canonical example with a side-effect setter:**
43+
`packages/utils.parser/src/operators.ts` β†’ `strictEquality` swaps the `==`
44+
and `!=` operator implementations when the option flips.
45+
46+
### Core `Options` class field (rare β€” only for `@tko/utils` intrinsics)
47+
48+
Add a field directly to the `Options` class in `packages/utils/src/options.ts`
49+
**only** when the option is intrinsic to `@tko/utils` itself β€” something that
50+
`@tko/utils` uses or enforces without knowing about any downstream package.
51+
52+
Current core fields include `templateSizeLimit`, `deferUpdates`,
53+
`useOnlyNativeEvents`, `onError`, `sanitizeHtmlTemplate`.
54+
55+
Never add a core-class field for an option that belongs to another package.
56+
Doing so forces `@tko/utils` to carry concepts from downstream packages and
57+
creates a wrong-way dependency.
58+
59+
## Quick test
60+
61+
> Is the option intrinsic to `@tko/utils`?
62+
63+
- Yes β†’ core class field.
64+
- No β†’ `defineOption` in the owning package.
65+
66+
In practice, new options are almost always the second case.

β€Žtko.io/public/llms.txtβ€Ž

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
- /agents/glossary.md β€” domain terms, concepts, packages
1818
- /agents/testing.md β€” how to run and verify TKO code
1919
- /agents/contract.md β€” state/binding/DOM architecture
20+
- /agents/options.md β€” ko.options.* β€” when to use defineOption vs core Options fields
2021
- /agents/verified-behaviors/index.md β€” test-backed behavior contracts
2122
- /agents/sample-tsx.html β€” minimal browser TSX scaffold
2223
- /examples/ β€” interactive self-contained HTML examples

0 commit comments

Comments
Β (0)