Bug: undefined within props is not faithfully deserialized from RSC payload

[lubieowoce]

React version: 18.3.0-canary-0cdfef19b-20231211

Related issues: #25687

Steps To Reproduce

Object variant:

1. Pass a prop={{ x: undefined }} from a server component to a client component

Array variant:

1. Pass a prop={[0, undefined, 2]} from a server component to a client component

Link to code example:

https://codesandbox.io/p/devbox/compassionate-benz-rj9dct

The current behavior

Object properties that are undefined aren't present in the deserialized object, and arrays end up with empty slots instead:

{ x: undefined }     ->  {}
[ 0, undefined, 2 ]  ->  [ 0, <1 empty item>, 2 ]

The expected behavior

If a property or array item is undefined, it should be deserialized as such. Object keys should remain present (with a value of undefined), and arrays shouldn't have empty slots.

Object properties aren't that big of a deal, although a key being present (but undefined) may have some semantic meaning in the user's data model, so it's still not ideal. But empty slots behave in very surprising ways with e.g. .map():

> [0,,2].map((x) => x ?? 'default')
[ 0, <1 empty item>, 2 ]

> [0,undefined,2].map((x) => x ?? 'default')
[ 0, 'default', 2 ]

this is likely to bite anyone who maps over an array in order to display something, which is common in react.

[lubieowoce]

This seems to stem directly from the behavior of JSON.parse if the reviver function returns undefined:

> function reviver(key, val) { return val === '$undefined') ? undefined : val }

> JSON.parse(`{ "x": "$undefined" }`, reviver)
{}
> JSON.parse(`[0, "$undefined", 2]`,reviver)
[ 0, <1 empty item>, 2 ]

One way i've found to work around the array part is to do something like this in the reviver:

function reviver(key, val) {
  if (Array.isArray(val)) { return [...val] }
  ...
}

Empty slots don't affect iteration, so this turns [0,,2] into [0,undefined,2]. It's a needless copy though.

[sebmarkbage]

This is a known issue and basically just worth the tradeoff for perf.

[sebmarkbage]

We could maybe check if this is an array when we find the '$undefined' value and if so add the array to a queue to be filled in later. It would make it really slow when it's used but at least it's only when there are undefineds instead of always.

Note that we also don't model empty slots separately from this concept so if we did, maybe we would need to also model empty slots separately.

[lubieowoce]

Made a little benchmark comparing a couple of these (though the inputs might not be very representative)
http://jsben.ch/D1aaP

Interestingly enough, in Firefox, a fixup queue that sets undefined where necessary (after the parse is finished) seems to outperform a simple reviver that just returns undefined! Maybe deleting properties (i.e. the default behavior of JSON.parse) ends up being slower than doing extra steps? idk. in Chrome, the queue ends up being a couple percentage points slower, so i guess it's not universal.

either way, it might not be a big deal, perf-wise? feels like in a decently-sized tree with few undefineds, the queue-based approach should cost very little, and generally be a blip at worst

[sebmarkbage]

The other issue is that since we don't emit the property from an object that has a value of undefined, for the same reason, we're still not really consistent about this.

[lubieowoce]

well, in the queue implementation, you kinda get both for free just by doing target[key] = undefined which handles both arrays and objects, right. i might be missing some subtlety, but it looks fine to me?

function parseWithFixup(input) {
  const fixups = [];
  function reviver(key, val) {
    if (val === "$undefined") {
      // we'll replace this value with an undefined at the end.
      fixups.push([this, key]);

      if (key === "") {
        // we might be at the top-level, i.e. visiting the root.
        // in case the whole string is "$undefined", we return undefined here --
        // we won't get the chance to replace it after.
        return undefined;
      }
      
      return null; // we'll replace it later, but just in case... it's close enough
    }

    return val;
  };

  const res = JSON.parse(input, reviver);
  
  for (let i = 0, len = fixups.length; i < len; i++) {
    const fixup = fixups[i];
    const target = fixup[0], key = fixup[1];
    target[key] = undefined;
  }

  return res;
}

> parseWithFixup(`[1, "$undefined", 2]`)
[ 1, undefined, 2 ]
> parseWithFixup(`{ "x": "$undefined" }`)
{ x: undefined }
> parseWithFixup(`"$undefined"`)
undefined

[github-actions]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

[eps1lon]

A failing test is already added in

react/packages/react-client/src/__tests__/ReactFlight-test.js

Lines 297 to 328 in e9ae2c8

	it('should transport undefined object values', async () => {
	function ServerComponent(props) {
	return 'prop' in props
	? `\`prop\` in props as '${props.prop}'`
	: '`prop` not in props';
	}
	const ClientComponent = clientReference(ServerComponent);
	const model = (
	<>
	<div>
	Server: <ServerComponent prop={undefined} />
	</div>
	<div>
	Client: <ClientComponent prop={undefined} />
	</div>
	</>
	);
	const transport = ReactNoopFlightServer.render(model);
	await act(async () => {
	ReactNoop.render(await ReactNoopFlightClient.read(transport));
	});
	expect(ReactNoop).toMatchRenderedOutput(
	<>
	<div>Server: `prop` in props as 'undefined'</div>
	<div>Client: `prop` in props as 'undefined'</div>
	</>,
	);
	});

in case somebody wants to try out different approaches.

I played with different fixes in the past but they all have different tradeoffs. There's one version that just uses the same queue method @lubieowoce proposed but also one that has constant space usage but more runtime checks.

Another argument for biting the perf bullet here is that in TypeScript you'd use the in operator for type narrowing when your props are a union of different object shapes. And TypeScript does differentiate at the type level between missing properties and ùndefined properties with [exactOptionalPropertiesTypes`](https://www.typescriptlang.org/tsconfig#exactOptionalPropertyTypes). Though empty array slots and map are a stronger argument I feel like.