feat: add stringifyAsync for async serialization#149
Conversation
Closes sveltejs#148 (async serialization portion). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 6c2caf3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
TooTallNate
left a comment
There was a problem hiding this comment.
Thanks for tackling this! The feature is a useful addition and the test coverage is great. However, before this can land I think the implementation needs to be reworked to avoid duplicating the entire body of stringify. As written, src/stringify-async.js is a near-verbatim copy of src/stringify.js with await sprinkled in — every future bug fix or new type added to stringify will silently get out of sync with stringifyAsync (the array sparse-encoding heuristic, the __proto__ guard, the typed-array handling, the Temporal types, stringify_primitive, etc. all now live in two places). I've left a few inline notes on specific things, but the headline ask is: please refactor so the two paths share a single implementation.
A few options worth considering:
- Make the core
flattenalways async-shaped. Have a singleflattenthat always usesawait, and implement syncstringifyby short-circuiting: if no Promises are encountered and no async reducers are used, thePromise<string>returned is already-resolved synchronously and you can unwrap it. Tricky but minimal. - Walk-then-stringify split (recommended). First, walk the graph to collect all Promises and run any async reducers, building a
Map<original, resolved>. Then call the existing synchronousstringifywith a synthetic reducer that consults the map. This is the smallest diff and isolates the async concern to a small wrapper. - Two-pass. Resolve all promises (including those returned from async reducers) in a first pass with
Promise.all, then hand the fully-resolved graph to existingstringify. As a bonus, this naturally fixes the parallelism issue flagged inline.
Option 2 or 3 would let you delete almost all of the new file. Holding off on approval until the duplication is addressed; a couple of correctness/perf notes are also flagged inline and should be looked at regardless of which approach you take.
| let p = 0; | ||
|
|
||
| /** @param {any} thing */ | ||
| async function flatten(thing) { |
There was a problem hiding this comment.
Blocking: code duplication. Everything from here to the end of this function is a copy of flatten in src/stringify.js with await added in a handful of places. That means:
- The sparse-array heuristic, the
__proto__guard, the Temporal cases, the typed-array subarray handling, and every error message now have to be maintained in two places. - Bug fixes and new type support added to
stringifywon't automatically apply tostringifyAsync(and vice-versa). There's no test that asserts the two implementations stay in lockstep beyond the fixture round-trip — and even that test will silently keep passing if both files happen to develop the same bug. - The
stringify_primitivehelper at the bottom of this file is also duplicated verbatim.
Please refactor so there's a single source of truth — see the top-level review for suggested approaches. Option 2 (walk-and-resolve, then delegate to existing stringify) would let this file shrink to ~50 lines and would naturally address the parallelism comment too.
| /** @param {any} thing */ | ||
| async function flatten(thing) { | ||
| // Resolve thenables/Promises before processing | ||
| if (thing !== null && typeof thing === 'object' && typeof thing.then === 'function') { |
There was a problem hiding this comment.
Blocking: Promise identity is never recorded.
await thing is performed before indexes.has(thing) is consulted on line 63, which means the Promise itself is never inserted into indexes. If the same Promise reference appears multiple times in the graph it will be await-ed every time it's encountered — for an already-resolved Promise this is just wasted work, but for a slow Promise that's referenced N times it's an N-way serial wait (because traversal is sequential — see other comment).
More importantly, this is a behavioral divergence from stringify's identity-based dedup contract. Users who carefully share a single Promise across the graph expecting it to round-trip as a shared reference will instead get the resolved value's identity-dedup behavior, which only kicks in if the resolved value is itself a non-primitive === to something previously seen. Two distinct Promise.resolve({a:1}) calls in the input will not dedup, even though sync-shared {a:1} does.
At minimum, please:
- Record the Promise in
indexesbefore awaiting (pointing at the same index that the resolved value will occupy), so subsequent encounters of the same Promise short-circuit. - Add a test for
stringifyAsync([p, p])wherepis a slow promise, asserting it is only awaited once (use a counter in the executor). - Document the dedup semantics for Promises in the README.
| async function flatten(thing) { | ||
| // Resolve thenables/Promises before processing | ||
| if (thing !== null && typeof thing === 'object' && typeof thing.then === 'function') { | ||
| thing = await thing; |
There was a problem hiding this comment.
Performance: independent Promises are awaited serially.
Because the traversal is a depth-first recursive await, two sibling Promises that aren't already started will run sequentially. Consider:
await stringifyAsync({
a: new Promise(r => setTimeout(() => r(1), 100)),
b: new Promise(r => setTimeout(() => r(2), 100))
});This takes ~200ms instead of ~100ms. The README example happens to work because fetch() starts the request eagerly, but Promises produced lazily inside reducers (or via new Promise(executor)) will be paid for one at a time.
A two-pass approach (collect all Promises during a sync walk → Promise.all → re-walk synchronously) avoids this entirely and is one more reason to pull the async concern out of the stringification logic. Not strictly blocking on its own, but combined with the duplication issue it's another argument for restructuring.
| if (indexes.has(thing)) return /** @type {number} */ (indexes.get(thing)); | ||
|
|
||
| const index = p++; | ||
| indexes.set(thing, index); |
There was a problem hiding this comment.
Minor: indexes.set(thing, index) here only registers the resolved value (since the original Promise was already overwritten by await on line 54). Worth a comment noting that thing here is guaranteed to be the resolved value, not the Promise — or, better, also record the original Promise (see the dedup comment) so the sharing contract is consistent with sync stringify.
| indexes.set(thing, index); | ||
|
|
||
| for (const { key, fn } of custom) { | ||
| let value = fn(thing); |
There was a problem hiding this comment.
Question: should reducers receive the Promise itself, or only the resolved value? As written, a reducer like MyPromise: (v) => v instanceof Promise && ... can never match because thing has already been awaited by the time we get here. That's probably the right default, but it deserves a line in the README — particularly because users coming from libraries like superjson may expect the opposite.
If you keep the current behavior, please also add a test for an async reducer that returns a thenable resolving to another thenable (which currently works, because flatten is recursively called on the awaited reducer output) — and a test for an async reducer that resolves to a function, asserting it produces the expected Cannot stringify a function error with a useful keys path.
| } | ||
| } | ||
|
|
||
| if (typeof thing === 'function') { |
There was a problem hiding this comment.
Nit: when this DevalueError is thrown for a function (or symbol) that came out of an awaited Promise, the keys array correctly reflects the path to the Promise, but thing is the resolved value. The error message is identical to the sync case, which is fine, but a user debugging "my fetch returned a function somehow" loses the breadcrumb that this came via a Promise. Optional: append (in resolved promise) to the message or keys when applicable.
| : `["RegExp",${stringify_string(source)}]`; | ||
| break; | ||
|
|
||
| case 'Array': { |
There was a problem hiding this comment.
Drive-by: the comments explaining the sparse-array heuristic that exist in stringify.js (lines 110-164) didn't get copied here. That heuristic is non-obvious and the comments are load-bearing — if for some reason this file survives the refactor, please port them over.
| ]); | ||
| assert.equal(result, stringify(['a', 'b'])); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Great that you're round-tripping every existing fixture through stringifyAsync — this catches the easy regressions. Please add tests for the cases flagged in the implementation:
- Same Promise referenced twice in the graph → assert it is only awaited once (use a counter in the executor).
- Two sibling Promises with
setTimeout→ assert total time ≈ max(durations), not sum (i.e., they ran in parallel). This will currently fail; it's the test that will guide the parallelism fix. - Promise resolving to a value that already exists elsewhere in the graph → assert dedup works.
- Cycle that includes a Promise (e.g.
obj.p = Promise.resolve(obj)) — there's no test for this and it's a tricky case worth pinning down. - Async reducer whose returned thenable rejects → assert the outer
stringifyAsyncrejects with that error.
| } catch (e) { | ||
| assert.equal(e.message, 'fail'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
thenable here is a synchronously-resolving thenable. Worth also testing:
- An asynchronously-resolving thenable.
- A thenable whose
thenthrows synchronously (which should reject the returned promise). - A thenable whose
thencallsreject.
The typeof thing.then === 'function' check in the implementation is a duck-type that will treat any object with a .then method as awaitable (incl. ones the user didn't intend to be awaited) — make sure there's a test demonstrating this is the intended contract.
| ```js | ||
| const stringified = await devalue.stringifyAsync(value, { | ||
| User: async (value) => value instanceof User && (await fetchUserData(value.id)) | ||
| }); |
There was a problem hiding this comment.
Worth documenting:
- Promises are awaited sequentially during traversal (or, ideally, fix that and document parallelism).
- Promise identity is not preserved — a Promise referenced twice does not produce two references to the same resolved value unless that resolved value is itself an identity-shared object. (Or, ideally, fix this and document that it is preserved.)
- Async reducers receive resolved values, never raw Promises.
- A rejection anywhere in the graph rejects the returned promise.
- There is intentionally no
parseAsync— async is a stringification-only concern (this is correct, but readers may wonder).
The section header reads ### stringifyAsyncandparse which is a little misleading — `parse` isn't doing anything async, it just happens to consume the output. Consider renaming to just `### `stringifyAsync and noting in prose that the output is consumed by the existing parse.
|
Opened #150 as a possible alternative approach |
|
Question: how necessary are async reducers? They add very significant overhead — the minute you have a single async reducer, you need to The example above... const json2 = await stringifyAsync(value, {
User: async (v) => v instanceof User && (await fetchUserData(v.id))
});...doesn't really need to be async — it could just be this, and the promise would be correctly serialized: const json2 = await stringifyAsync(value, {
User: (v) => v instanceof User && fetchUserData(v.id)
});This isn't just drastically more efficient, it's also less code for the user to write. Are there cases where the |
|
@Rich-Harris Do you reckon it would be too weird to differentiate between That could mean something like using I think it'd be a little surprising as a user since they are effectively "the same," but on the other hand it'd only be used as a hint to trigger an optimized path—treating a |
|
Hmm. I don't think they would be totally equivalent — surely you'd get different results for these? class Foo {
constructor(promise) {
this.promise = promise;
}
}
const foo = new Foo(Promise.resolve(false));
const a = await devalue.stringifyAsync(foo, {
Foo: (value) => value instanceof Foo && value.promise
});
const b = await devalue.stringifyAsync(foo, {
Foo: async (value) => value instanceof Foo && value.promise
});In the first case you would get |
|
closing in favour of #150 |
Preview 👀
Async version of
stringifythat resolves promises in the value graph and supports async reducers. Returns aPromise<string>in the same format asstringify, soparseandunflattenwork unchanged.Addresses #148 (async serialization portion).