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

[Flight] add support for Lazy components in Flight server #24068

Merged
merged 3 commits into from
Mar 10, 2022
+236 −14
Merged

[Flight] add support for Lazy components in Flight server #24068

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e6cdb3
[Flight] add support for Lazy components in Flight server
gnoff Mar 9, 2022
6b9c8c6
Support Lazy elements
gnoff Mar 10, 2022
a4329f4
add lazy initialization to resolveModelToJson
gnoff Mar 10, 2022
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
133 changes: 133 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 @@ -138,6 +138,139 @@ describe('ReactFlight', () => {
expect(ReactNoop).toMatchRenderedOutput(<span>Hello, Seb Smith</span>);
});

it('can render a lazy component as a shared component on the server', async () => {
function SharedComponent({text}) {
return (
<div>
shared<span>{text}</span>
</div>
);
}

let load = null;
const loadSharedComponent = () => {
return new Promise(res => {
load = () => res({default: SharedComponent});
});
};

const LazySharedComponent = React.lazy(loadSharedComponent);

function ServerComponent() {
return (
<React.Suspense fallback={'Loading...'}>
<LazySharedComponent text={'a'} />
</React.Suspense>
);
}

const transport = ReactNoopFlightServer.render(<ServerComponent />);

act(() => {
const rootModel = ReactNoopFlightClient.read(transport);
ReactNoop.render(rootModel);
});
expect(ReactNoop).toMatchRenderedOutput('Loading...');
await load();

act(() => {
const rootModel = ReactNoopFlightClient.read(transport);
ReactNoop.render(rootModel);
});
expect(ReactNoop).toMatchRenderedOutput(
<div>
shared<span>a</span>
</div>,
);
});

it('can render a lazy element', async () => {
function SharedComponent({text}) {
return (
<div>
shared<span>{text}</span>
</div>
);
}

let load = null;

const lazySharedElement = React.lazy(() => {
return new Promise(res => {
load = () => res({default: <SharedComponent text={'a'} />});
});
});

function ServerComponent() {
return (
<React.Suspense fallback={'Loading...'}>
{lazySharedElement}
</React.Suspense>
);
}

const transport = ReactNoopFlightServer.render(<ServerComponent />);

act(() => {
const rootModel = ReactNoopFlightClient.read(transport);
ReactNoop.render(rootModel);
});
expect(ReactNoop).toMatchRenderedOutput('Loading...');
await load();

act(() => {
const rootModel = ReactNoopFlightClient.read(transport);
ReactNoop.render(rootModel);
});
expect(ReactNoop).toMatchRenderedOutput(
<div>
shared<span>a</span>
</div>,
);
});

it('can render a lazy module reference', async () => {
function ClientComponent() {
return <div>I am client</div>;
}

const ClientComponentReference = moduleReference(ClientComponent);

let load = null;
const loadClientComponentReference = () => {
return new Promise(res => {
load = () => res({default: ClientComponentReference});
});
};

const LazyClientComponentReference = React.lazy(
loadClientComponentReference,
);

function ServerComponent() {
return (
<React.Suspense fallback={'Loading...'}>
<LazyClientComponentReference />
</React.Suspense>
);
}

const transport = ReactNoopFlightServer.render(<ServerComponent />);

act(() => {
const rootModel = ReactNoopFlightClient.read(transport);
ReactNoop.render(rootModel);
});
expect(ReactNoop).toMatchRenderedOutput('Loading...');
await load();

act(() => {
const rootModel = ReactNoopFlightClient.read(transport);
ReactNoop.render(rootModel);
});
expect(ReactNoop).toMatchRenderedOutput(<div>I am client</div>);
});

it('should error if a non-serializable value is passed to a host component', () => {
function EventHandlerProp() {
return (
Expand Down
46 changes: 32 additions & 14 deletions packages/react-server/src/ReactFlightServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,21 @@ function attemptResolveElement(
return [REACT_ELEMENT_TYPE, type, key, props];
}
switch (type.$$typeof) {
case REACT_LAZY_TYPE: {
const payload = type._payload;
const init = type._init;
const wrappedType = init(payload);
return attemptResolveElement(wrappedType, key, ref, props);
}
case REACT_FORWARD_REF_TYPE: {
const render = type.render;
return render(props, undefined);
}
case REACT_ELEMENT_TYPE: {
// this can happen when a lazy component resolves to an element instead of
// a Component.
return attemptResolveElement(type.type, type.key, type.ref, type.props);
}
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 the solution is to just remove this and then let it fall through to the error case. You could also put custom error messages at the bottom of this case if you want to explain in plain words to the user what's wrong.

Semantically this is not right since it's dropping key, ref and props from the parent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, the need for this went away when I added lazy resolution to resolveModelToJSON

case REACT_MEMO_TYPE: {
return attemptResolveElement(type.type, key, ref, props);
}
Expand Down Expand Up @@ -452,10 +463,6 @@ export function resolveModelToJSON(
switch (value) {
case REACT_ELEMENT_TYPE:
return '$';
case REACT_LAZY_TYPE:
throw new Error(
'React Lazy Components are not yet supported on the server.',
);
Comment on lines -455 to -458
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sebmarkbage

In testing I could not ever get this case to throw. the value at this point is a React.Element and not a symbol so it ends up throwing in attemptResolveElement instead. If I am missing a use case that did hit this path that needs to be covered by tests let me know

Copy link
Collaborator

@sebmarkbage sebmarkbage Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You can now use lazy in the position of React "nodes" (i.e. children). That's actually what Flight itself does on the client.

const element = React.lazy(async () => {
  return <Component prop="prop" />;
});
...
<div>{element}</div>

That should probably ideally work in Server Components too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also technically, you could pass just the symbol. <Component foo={lazyComponent.$$typeof} />

Regardless, if we see it, it's probably a bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add support for it, thanks

Copy link
Collaborator Author

@gnoff gnoff Mar 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What should we do if someone does this

const ElementDisguisedAsAComponent = React.lazy(async () => {
  return <Component prop={"original"} />;
});
...
<div><ElementDisguisedAsAComponent prop={"new"}</div>

feels like that should error in Dev and in prod we should render the actual element throwing away the "new" prop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It works the way I said it should in prod already and there is no conditional, we could add the conditional in dev only

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What happens in the client when you do this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, the opposite is supported

const componentDisguisedAsElement = React.lazy(async () => {
  return Component;
});
...
<div>{componentDisguisedAsElement}</div>

it will render like you did i.e. empty props. I'm thinking this should also error in dev

Copy link
Collaborator

Choose a reason for hiding this comment

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

The componentDisguisedAsElement case seems like it should hit this case?

https://github.com/facebook/react/blob/main/packages/react-server/src/ReactFlightServer.js#L635

Regardless, it should behave as if there was no lazy around it.

Copy link
Collaborator Author

@gnoff gnoff Mar 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

that's a good hueristic, I've updated tests and behavior to have these match. It'll be an error to use lazy elements as Components and vice versa just like without lazy

}

if (__DEV__) {
Expand All @@ -477,23 +484,34 @@ export function resolveModelToJSON(
while (
typeof value === 'object' &&
value !== null &&
(value: any).$$typeof === REACT_ELEMENT_TYPE
((value: any).$$typeof === REACT_ELEMENT_TYPE ||
(value: any).$$typeof === REACT_LAZY_TYPE)
) {
if (__DEV__) {
if (isInsideContextValue) {
console.error('React elements are not allowed in ServerContext');
}
}
// TODO: Concatenate keys of parents onto children.
const element: React$Element<any> = (value: any);

try {
// Attempt to render the server component.
value = attemptResolveElement(
element.type,
element.key,
element.ref,
element.props,
);
switch ((value: any).$$typeof) {
case REACT_ELEMENT_TYPE: {
// TODO: Concatenate keys of parents onto children.
const element: React$Element<any> = (value: any);
// Attempt to render the server component.
value = attemptResolveElement(
element.type,
element.key,
element.ref,
element.props,
);
break;
}
case REACT_LAZY_TYPE: {
value = attemptResolveElement(value, null, null, {});
break;
}
}
} catch (x) {
if (typeof x === 'object' && x !== null && typeof x.then === 'function') {
// Something suspended, we'll need to create a new segment and resolve it later.
Expand Down