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.

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

Encode throwing server components as lazy throwing references #20217

Merged
merged 1 commit into from
Nov 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
64 changes: 64 additions & 0 deletions packages/react-client/src/__tests__/ReactFlight-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ let ReactNoop;
let ReactNoopFlightServer;
let ReactNoopFlightClient;
let ErrorBoundary;
let NoErrorExpected;

describe('ReactFlight', () => {
beforeEach(() => {
Expand Down Expand Up @@ -47,6 +48,26 @@ describe('ReactFlight', () => {
return this.props.children;
}
};

NoErrorExpected = class extends React.Component {
state = {hasError: false, error: null};
static getDerivedStateFromError(error) {
return {
hasError: true,
error,
};
}
componentDidMount() {
expect(this.state.error).toBe(null);
expect(this.state.hasError).toBe(false);
}
render() {
if (this.state.hasError) {
return this.state.error.message;
}
return this.props.children;
}
};
});

function moduleReference(value) {
Expand Down Expand Up @@ -164,6 +185,49 @@ describe('ReactFlight', () => {
});
});

it('should trigger the inner most error boundary inside a client component', () => {
function ServerComponent() {
throw new Error('This was thrown in the server component.');
}

function ClientComponent({children}) {
// This should catch the error thrown by the server component, even though it has already happened.
// We currently need to wrap it in a div because as it's set up right now, a lazy reference will
// throw during reconciliation which will trigger the parent of the error boundary.
// This is similar to how these will suspend the parent if it's a direct child of a Suspense boundary.
// That's a bug.
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Nov 11, 2020

Choose a reason for hiding this comment

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

@acdlite This is related to the problem were throwing during reconciliation triggers the nearest boundary. These are lazy elements. Not lazy types. So they throw here and not during beginWork. Since we don't have the key and stuff necessarily to complete the reconciliation. https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactChildFiber.new.js#L1251

Currently this affects Suspense boundaries in Flight too where they might suspend the parent's parent boundary instead of the direct parent.

return (
<ErrorBoundary expectedMessage="This was thrown in the server component.">
<div>{children}</div>
</ErrorBoundary>
);
}

const ClientComponentReference = moduleReference(ClientComponent);

function Server() {
return (
<ClientComponentReference>
<ServerComponent />
</ClientComponentReference>
);
}

const data = ReactNoopFlightServer.render(<Server />);

function Client({transport}) {
return ReactNoopFlightClient.read(transport);
}

act(() => {
ReactNoop.render(
<NoErrorExpected>
<Client transport={data} />
</NoErrorExpected>,
);
});
});

it('should warn in DEV if a toJSON instance is passed to a host component', () => {
expect(() => {
const transport = ReactNoopFlightServer.render(
Expand Down
9 changes: 7 additions & 2 deletions packages/react-server/src/ReactFlightServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -409,8 +409,13 @@ export function resolveModelToJSON(
x.then(ping, ping);
return serializeByRefID(newSegment.id);
} else {
// Something errored. Don't bother encoding anything up to here.
throw x;
// Something errored. We'll still send everything we have up until this point.
// We'll replace this element with a lazy reference that throws on the client
// once it gets rendered.
request.pendingChunks++;
const errorId = request.nextChunkId++;
emitErrorChunk(request, errorId, x);
return serializeByRefID(errorId);
}
}
}
Expand Down