Skip to content

feat: add stringifyAsync for async serialization#149

Closed
gr2m wants to merge 1 commit into
sveltejs:mainfrom
gr2m:async-serialization
Closed

feat: add stringifyAsync for async serialization#149
gr2m wants to merge 1 commit into
sveltejs:mainfrom
gr2m:async-serialization

Conversation

@gr2m

@gr2m gr2m commented Apr 14, 2026

Copy link
Copy Markdown

Preview 👀

Async version of stringify that resolves promises in the value graph and supports async reducers. Returns a Promise<string> in the same format as stringify, so parse and unflatten work unchanged.

Addresses #148 (async serialization portion).

import { stringifyAsync, parse } from 'devalue';

// Promises in the value graph are awaited automatically
const json = await stringifyAsync({
  sync: 'hello',
  async: Promise.resolve('world')
});
parse(json); // { sync: 'hello', async: 'world' }

// Reducers can be async
const json2 = await stringifyAsync(value, {
  User: async (v) => v instanceof User && (await fetchUserData(v.id))
});

Closes sveltejs#148 (async serialization portion).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Apr 14, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 6c2caf3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
devalue Minor

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

@gr2m gr2m marked this pull request as ready for review April 14, 2026 21:11

@TooTallNate TooTallNate left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Make the core flatten always async-shaped. Have a single flatten that always uses await, and implement sync stringify by short-circuiting: if no Promises are encountered and no async reducers are used, the Promise<string> returned is already-resolved synchronously and you can unwrap it. Tricky but minimal.
  2. 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 synchronous stringify with a synthetic reducer that consults the map. This is the smallest diff and isolates the async concern to a small wrapper.
  3. 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 existing stringify. 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.

Comment thread src/stringify-async.js
let p = 0;

/** @param {any} thing */
async function flatten(thing) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 stringify won't automatically apply to stringifyAsync (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_primitive helper 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.

Comment thread src/stringify-async.js
/** @param {any} thing */
async function flatten(thing) {
// Resolve thenables/Promises before processing
if (thing !== null && typeof thing === 'object' && typeof thing.then === 'function') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Record the Promise in indexes before awaiting (pointing at the same index that the resolved value will occupy), so subsequent encounters of the same Promise short-circuit.
  2. Add a test for stringifyAsync([p, p]) where p is a slow promise, asserting it is only awaited once (use a counter in the executor).
  3. Document the dedup semantics for Promises in the README.

Comment thread src/stringify-async.js
async function flatten(thing) {
// Resolve thenables/Promises before processing
if (thing !== null && typeof thing === 'object' && typeof thing.then === 'function') {
thing = await thing;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/stringify-async.js
if (indexes.has(thing)) return /** @type {number} */ (indexes.get(thing));

const index = p++;
indexes.set(thing, index);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/stringify-async.js
indexes.set(thing, index);

for (const { key, fn } of custom) {
let value = fn(thing);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/stringify-async.js
}
}

if (typeof thing === 'function') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/stringify-async.js
: `["RegExp",${stringify_string(source)}]`;
break;

case 'Array': {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread test/index.test.js
]);
assert.equal(result, stringify(['a', 'b']));
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 stringifyAsync rejects with that error.

Comment thread test/index.test.js
} catch (e) {
assert.equal(e.message, 'fail');
}
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thenable here is a synchronously-resolving thenable. Worth also testing:

  • An asynchronously-resolving thenable.
  • A thenable whose then throws synchronously (which should reject the returned promise).
  • A thenable whose then calls reject.

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.

Comment thread README.md
```js
const stringified = await devalue.stringifyAsync(value, {
User: async (value) => value instanceof User && (await fetchUserData(value.id))
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth documenting:

  1. Promises are awaited sequentially during traversal (or, ideally, fix that and document parallelism).
  2. 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.)
  3. Async reducers receive resolved values, never raw Promises.
  4. A rejection anywhere in the graph rejects the returned promise.
  5. 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.

@Rich-Harris Rich-Harris mentioned this pull request Apr 28, 2026
@Rich-Harris

Copy link
Copy Markdown
Member

Opened #150 as a possible alternative approach

@Rich-Harris

Copy link
Copy Markdown
Member

Question: how necessary are async reducers? They add very significant overhead — the minute you have a single async reducer, you need to await every single value (that doesn't have a sentinel value representation, like UNDEFINED) as you walk the tree.

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 instanceof check itself would need to be behind an await? If not I would strongly advocate for sync reducers

@mrkishi

mrkishi commented Apr 29, 2026

Copy link
Copy Markdown
Member

@Rich-Harris Do you reckon it would be too weird to differentiate between AsyncFunction reducers vs Promise-returning reducers?

That could mean something like using custom.push({ key, fn: reducers[key], async: reducers[key].constructor.name === 'AsyncFunction' }) and switching between sync/async versions of run/flatten if any reducer is async, and only actually awaiting async reducers.

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 Promise-returning reducer as an async one would ultimately lead to the same serialized value, only slower.

@Rich-Harris

Copy link
Copy Markdown
Member

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 [["Foo",1],false], in the second you would get a 'cannot stringify' error. That feels like too sharp an edge to me — at the very least it's not quite true (IIUC) that the only difference between otherwise-equivalent async and non-async functions is whether the fast path applies

@Rich-Harris

Copy link
Copy Markdown
Member

closing in favour of #150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants