Flush BatchingRemoteConnection on a microtask by default#604
Flush BatchingRemoteConnection on a microtask by default#604justinhenricks merged 1 commit intomainfrom
Conversation
robin-drexler
left a comment
There was a problem hiding this comment.
would really love to get @developit or @henrytao-me's 👀 on this
| @@ -0,0 +1,21 @@ | |||
| --- | |||
| '@remote-dom/core': minor | |||
There was a problem hiding this comment.
I almost wonder if that should be a breaking change. I'm leaning more towards no, but the thought has crossed my mind.
There was a problem hiding this comment.
It feels more like a bug fix to me
There was a problem hiding this comment.
yeah, I think if something were to break with this change that would be unexpected
There was a problem hiding this comment.
Minor request: In the PR description, I suggest adding a concrete example of how focusing the DOM after an error won't work since the DOM is stale. This example will concisely demonstrate the bug you're fixing.
An AI review also caught a minor issue worth looking into.
Otherwise the fix looks good ✨
AI review
Reviewed with Claude Opus 4.7 via pi.
Explicit per-test setup
- Consider splitting "flushes mutations" into three focused tests: (a)
.flush()drains synchronously, (b).flush()cancels the pending microtask (no double-fire), (c) a subsequentmutate()schedules a fresh batch. Scenario (b) currently has no dedicated coverage.
kumar303
left a comment
There was a problem hiding this comment.
^ whoops, meant to approve
00a5ce8 to
7177edb
Compare
|
Thanks @kumar303 updated the PR description and addressed the feedback, as well as the AI note on splitting up the tests. |
Summary
Change the default
batchfunction used byBatchingRemoteConnectionto flush queued mutations on a microtask (viaqueueMicrotask, falling back toPromise.resolve().then(...)) instead of a macrotask viaMessageChannel/setTimeout.Why
When the remote side runs logic like:
...and the host runs logic like:
...over an
@remote-ui/rpc-style channel, the timeline under the old macrotask default is:setErrorqueues a remote-dom mutation (scheduled on a macrotask viaMessageChannel)awaitresolves via the microtask queuefocusFirstErrorruns against stale DOM and finds 0 targetsAwaiting yields to the microtask queue but not the macrotask queue, so the RPC response always beats the mutation to the host.
In production, this is why checkout wouldn't auto-scroll to a buyer's invalid field when a blocking extension set an error: the auto-scroll ran before the error mutation landed, and there was nothing in the DOM to scroll to. We shipped the fix downstream in checkout-web (shop/world#604053, internal) by passing a custom
batchoption, but the macrotask default is surprising and easy to miss, so this patches it upstream.Scheduling the flush on a microtask guarantees mutations land on the host before any
awaited RPC response resolves.Changes
BatchingRemoteConnection's defaultbatchnow usesqueueMicrotask, with aPromise.resolve().then(...)fallback.batchoption.waitForNextTask→waitForNextMicrotask.awaited promise resolves.queueMicrotaskis used.Promise.resolve().then(...)fallback whenqueueMicrotaskis unavailable.minorchangeset.Backwards compatibility
This is a behavioral change to the default, hence the minor bump. Consumers who depended on macrotask timing can restore it with a custom
batchoption (documented in both the changeset and the README).