fix: coalesce scan() into a single rAF to avoid paint delay on bulk DOM mutations#344
fix: coalesce scan() into a single rAF to avoid paint delay on bulk DOM mutations#344
Conversation
…OM mutations Replace per-element WeakMap+rAF deduplication with a shared Set of pending elements and a single requestAnimationFrame timer. When many elements are added in one frame (e.g. framework list rendering), only one rAF callback now runs before paint instead of one per element. Fixes #343 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses performance regressions in lazyDefine by coalescing scan() work into a single requestAnimationFrame callback per frame, reducing pre-paint rAF flooding during bulk DOM insertions (Fixes #343).
Changes:
- Replaced per-element rAF deduplication (
WeakMap) with a sharedSetof pending elements and a single rAF timer. - Updated
scan()to process all pending elements in one rAF callback. - Added a test asserting that bulk element insertion does not schedule one rAF per element.
Show a summary per file
| File | Description |
|---|---|
src/lazy-define.ts |
Coalesces multiple scan() calls into a single rAF callback using a shared pending set. |
test/lazy-define.ts |
Adds coverage to ensure bulk-added elements don’t cause excessive rAF scheduling. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
| function scan(element: ElementLike) { | ||
| cancelAnimationFrame(timers.get(element) || 0) | ||
| timers.set( | ||
| element, | ||
| requestAnimationFrame(() => { | ||
| pendingElements.add(element) | ||
| if (scanTimer != null) return | ||
| scanTimer = requestAnimationFrame(() => { | ||
| scanTimer = null | ||
| if (!dynamicElements.size) { | ||
| pendingElements.clear() | ||
| return | ||
| } | ||
| for (const el of pendingElements) { | ||
| for (const tagName of dynamicElements.keys()) { | ||
| const child: Element | null = | ||
| element instanceof Element && element.matches(tagName) ? element : element.querySelector(tagName) | ||
| const child: Element | null = el instanceof Element && el.matches(tagName) ? el : el.querySelector(tagName) | ||
| if (customElements.get(tagName) || child) { | ||
| const strategyName = (child?.getAttribute('data-load-on') || 'ready') as keyof typeof strategies | ||
| const strategy = strategyName in strategies ? strategies[strategyName] : strategies.ready | ||
| // eslint-disable-next-line github/no-then | ||
| for (const cb of dynamicElements.get(tagName) || []) strategy(tagName).then(cb) | ||
| dynamicElements.delete(tagName) | ||
| timers.delete(element) | ||
| } | ||
| } | ||
| }) | ||
| ) | ||
| } | ||
| pendingElements.clear() | ||
| }) |
There was a problem hiding this comment.
pendingElements.clear() runs at the end of the rAF callback, but scanTimer is set to null at the start of that callback. If scan() is invoked while this rAF callback is executing, it will schedule a new rAF and add elements into pendingElements, but those newly-added elements will be cleared by the currently-running callback, so the next rAF runs with an empty set and the scan is effectively dropped. To avoid losing scans, snapshot+clear pendingElements at the start of the rAF callback (process the snapshot), leaving any new elements added during processing for the next scheduled rAF.
| for (const el of pendingElements) { | ||
| for (const tagName of dynamicElements.keys()) { | ||
| const child: Element | null = | ||
| element instanceof Element && element.matches(tagName) ? element : element.querySelector(tagName) | ||
| const child: Element | null = el instanceof Element && el.matches(tagName) ? el : el.querySelector(tagName) | ||
| if (customElements.get(tagName) || child) { | ||
| const strategyName = (child?.getAttribute('data-load-on') || 'ready') as keyof typeof strategies | ||
| const strategy = strategyName in strategies ? strategies[strategyName] : strategies.ready | ||
| // eslint-disable-next-line github/no-then | ||
| for (const cb of dynamicElements.get(tagName) || []) strategy(tagName).then(cb) | ||
| dynamicElements.delete(tagName) | ||
| timers.delete(element) | ||
| } | ||
| } | ||
| }) | ||
| ) | ||
| } | ||
| pendingElements.clear() |
There was a problem hiding this comment.
Within the rAF callback, once dynamicElements becomes empty (after dynamicElements.delete(tagName)), the outer loop still iterates over all remaining pendingElements, doing no useful work. For large DOM mutation batches this adds avoidable work before paint. Consider short-circuiting out of the loops when dynamicElements.size === 0 (e.g., a labeled break) to keep the rAF callback bounded by the number of tagNames actually pending.
| const rafSpy = spy(window, 'requestAnimationFrame') | ||
| const callsBefore = rafSpy.callCount | ||
|
|
||
| await fixture(html` | ||
| <div> | ||
| <coalesce-test-element></coalesce-test-element> | ||
| <coalesce-test-element></coalesce-test-element> | ||
| <coalesce-test-element></coalesce-test-element> | ||
| <coalesce-test-element></coalesce-test-element> | ||
| <coalesce-test-element></coalesce-test-element> | ||
| <coalesce-test-element></coalesce-test-element> | ||
| <coalesce-test-element></coalesce-test-element> | ||
| <coalesce-test-element></coalesce-test-element> | ||
| <coalesce-test-element></coalesce-test-element> | ||
| <coalesce-test-element></coalesce-test-element> | ||
| </div> | ||
| `) | ||
|
|
||
| await animationFrame() | ||
|
|
||
| const rafCallsFromScan = rafSpy.callCount - callsBefore | ||
| rafSpy.restore() | ||
|
|
||
| // Should use at most a few rAF calls, not one per element | ||
| expect(rafCallsFromScan).to.be.lessThan(5) | ||
| expect(onDefine).to.be.callCount(1) | ||
| }) |
There was a problem hiding this comment.
The requestAnimationFrame spy is only restored on the happy path. If fixture() or animationFrame() throws, rafSpy.restore() won't run and will leak the spy into later tests. Wrap the body in a try/finally (or use Sinon sandbox) to guarantee restore.
Summary
Fixes #343
When many elements are added to the DOM in a single frame (e.g. a framework rendering a list), the
scan()function inlazy-define.tswas scheduling a separaterequestAnimationFramecallback for each element. Since all rAF callbacks run before the browser paints, this delayed the first paint proportionally to the number of added elements.Changes
Replaced the per-element
WeakMap+requestAnimationFramededuplication with:Set<ElementLike>that collects pending elementsrequestAnimationFrametimer that processes all pending elements in one callbackThis ensures that no matter how many elements are added in a single frame, only one rAF callback runs before paint.
Testing
Added a test that verifies multiple elements added at once are coalesced into minimal rAF calls rather than one per element. All existing tests continue to pass.