Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Flight] Add support for returning undefined from render #26349

Merged
merged 4 commits into from
Mar 9, 2023
Merged

[Flight] Add support for returning undefined from render #26349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f9af385
Add test for rendering undefined
eps1lon Mar 8, 2023
b263419
[Flight] Support returning undefined
eps1lon Mar 8, 2023
489ecc4
Regression test for empty fragments
eps1lon Mar 8, 2023
9732259
$U -> $undefined
eps1lon Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/react-client/src/ReactFlightClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,11 @@ export function parseModelString(
throw chunk.reason;
}
}
case 'u': {
// matches "$undefined"
// Special encoding for `undefined` which can't be serialized as JSON otherwise.
return undefined;
}
default: {
// We assume that anything else is a reference ID.
const id = parseInt(value.substring(1), 16);
Expand Down
32 changes: 32 additions & 0 deletions packages/react-client/src/__tests__/ReactFlight-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,38 @@ describe('ReactFlight', () => {
expect(ReactNoop).toMatchRenderedOutput(<span>ABC</span>);
});

it('can render undefined', async () => {
function Undefined() {
return undefined;
}

const model = <Undefined />;

const transport = ReactNoopFlightServer.render(model);

await act(async () => {
ReactNoop.render(await ReactNoopFlightClient.read(transport));
});

expect(ReactNoop).toMatchRenderedOutput(null);
});

it('can render an empty fragment', async () => {
function Empty() {
return <React.Fragment />;
}

const model = <Empty />;

const transport = ReactNoopFlightServer.render(model);

await act(async () => {
ReactNoop.render(await ReactNoopFlightClient.read(transport));
});

expect(ReactNoop).toMatchRenderedOutput(null);
});

it('can render a lazy component as a shared component on the server', async () => {
function SharedComponent({text}) {
return (
Expand Down
15 changes: 10 additions & 5 deletions packages/react-server/src/ReactFlightServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ export type ReactClientValue =
| number
| symbol
| null
| void
| Iterable<ReactClientValue>
| Array<ReactClientValue>
| ReactClientObject
Expand Down Expand Up @@ -546,6 +547,10 @@ function serializeProviderReference(name: string): string {
return '$P' + name;
}

function serializeUndefined(): string {
return '$undefined';
}

function serializeClientReference(
request: Request,
parent:
Expand Down Expand Up @@ -1134,14 +1139,14 @@ export function resolveModelToJSON(
return escapeStringValue(value);
}

if (
typeof value === 'boolean' ||
typeof value === 'number' ||
typeof value === 'undefined'
) {
if (typeof value === 'boolean' || typeof value === 'number') {
return value;
}

if (typeof value === 'undefined') {
return serializeUndefined();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This serializes properties on object that have the value undefined. This is technically more correct since it has the property but can we live with it being a missing property instead to save bytes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checking for property existence with in is not uncommon in TypeScript since it plays well with type-narrowing. TypeScript also checks for correct usage of missing properties and properties that are undefined if exactOptionalPropertyTypes is enabled.

Also not sure how people will feel about inconsistent props if being passed from server -> client vs server->serer and client->client.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe revisit this behind a flag once Server Components are more widely adopted?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'd have to start with the less correct behavior and then make it correct rather than the other way around. It'll be harder to break later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok saying that we just waste the bytes here though. You probably shouldn't be using so many optional undefined props anyway. Personally I always use null instead anyway.

}

if (typeof value === 'function') {
if (isClientReference(value)) {
return serializeClientReference(request, parent, key, (value: any));
Expand Down