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] Update stale blocked values in createModelResolver #28669

Merged
merged 6 commits into from
Apr 1, 2024
Merged

[Flight] Update stale blocked values in createModelResolver #28669

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6e92c9f
Add a test for issue #28595
unstubbable Mar 20, 2024
4099282
Wrap `Posts` in `Suspense` to differentiate test expectations
unstubbable Mar 23, 2024
5d1bdea
Enhance test to show that b835af277 is not a solution
unstubbable Mar 28, 2024
c578cff
Update stale blocked values in `createModelResolver`
unstubbable Mar 29, 2024
7d5ad2e
Never outline element props in Flight Server
unstubbable Mar 30, 2024
32c5b0b
Cast `value` to `any`
unstubbable Mar 30, 2024
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
19 changes: 19 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 @@ -584,6 +584,25 @@ function createModelResolver<T>(
}
return value => {
parentObject[key] = value;

// If this is a tuple that's representing a react element, and the props
// were resolved (4th item, key '3'), the props of `blocked.value` must be
// updated.
if (
Array.isArray(parentObject) &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an interesting observation. For any parentObject where we later might replace it with another object, this problem occurs. I guess that can really just be React Elements for now.

https://github.com/facebook/react/blob/main/packages/react-client/src/ReactFlightClient.js#L1440-L1442

It seems like in principle it could happen to any of the keys inside of it though. Not just the props.

Also, it seems like in theory it should be able to happen even even if the element is not at the root. I think it's problematic to assume that if the parent object is representing the object that's blocked at the root. If the blocked object is an element that itself has further objects below which itself happens to have an array as the parent object then this would incorrectly match that case. So this doesn't seem safe.

At first, I was confused how this could happen but it's really an artifact of the other problem that the props get outlined separately from the element because when we start outlining an object we've seen everything inside of it before which causes us to outline everything inside of it too which is inefficient because we might not ever see them again outside of this particular object. Which happens to props and elements.

This should ideally have some more clever strategy.

However, one thing we could do is mark props objects inside elements in the outlining as never outlined. E.g. give it id -2 or something. That way they're never outline independently from the element that they're apart of. That way this particular scenario doesn't happen. (Although ideally the parser would be resilient to it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of not outlining the props. The condition for updating them in the blocked element felt brittle to me. I've implemented the suggested change in 7d5ad2e.

Copy link
Contributor Author

@unstubbable unstubbable Mar 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What this doesn't do (yet), is prevent the objects inside of the props from being outlined. Is this something you would want to see being done too? To fix the issue, this wouldn't be needed. It might maybe be useful to keep outlining them when the same object is passed to different elements as part of the props. Although it would of course be even better to detect exactly this case, and only then outline them. Not sure if we want to add more complexity for this though.

key === '3' &&
blocked.value &&
blocked.value.$$typeof === REACT_ELEMENT_TYPE
) {
blocked.value.props = value;
}

// If this is the root object for a model reference, where `blocked.value`
// is a stale `null`, the resolved value can be used directly.
if (key === '' && blocked.value === null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is clever. I think this hold true because if the root is blocked, there can't be any other objects that are also blocked. At least for now. Might not hold in the future but should be ok for now at least.

blocked.value = value;
}

blocked.deps--;
if (blocked.deps === 0) {
if (chunk.status !== BLOCKED) {
Expand Down
104 changes: 104 additions & 0 deletions packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,6 +812,110 @@ describe('ReactFlightDOM', () => {
expect(reportedErrors).toEqual([]);
});

it('should handle streaming async server components', async () => {
const reportedErrors = [];

const Row = async ({current, next}) => {
const chunk = await next;

if (chunk.done) {
return chunk.value;
}

return (
<Suspense fallback={chunk.value}>
<Row current={chunk.value} next={chunk.next} />
</Suspense>
);
};

function createResolvablePromise() {
let _resolve, _reject;

const promise = new Promise((resolve, reject) => {
_resolve = resolve;
_reject = reject;
});

return {promise, resolve: _resolve, reject: _reject};
}

function createSuspendedChunk(initialValue) {
const {promise, resolve, reject} = createResolvablePromise();

return {
row: (
<Suspense fallback={initialValue}>
<Row current={initialValue} next={promise} />
</Suspense>
),
resolve,
reject,
};
}

function makeDelayedText() {
const {promise, resolve, reject} = createResolvablePromise();
async function DelayedText() {
const data = await promise;
return <div>{data}</div>;
}
return [DelayedText, resolve, reject];
}

const [Posts, resolvePostsData] = makeDelayedText();
const [Photos, resolvePhotosData] = makeDelayedText();
const suspendedChunk = createSuspendedChunk(<p>loading</p>);
const {writable, readable} = getTestStream();
const {pipe} = ReactServerDOMServer.renderToPipeableStream(
suspendedChunk.row,
webpackMap,
{
onError(error) {
reportedErrors.push(error);
},
},
);
pipe(writable);
const response = ReactServerDOMClient.createFromReadableStream(readable);
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);

function ClientRoot() {
return use(response);
}

await act(() => {
root.render(<ClientRoot />);
});

expect(container.innerHTML).toBe('<p>loading</p>');

const donePromise = createResolvablePromise();

const value = (
<Suspense fallback={<p>loading posts and photos</p>}>
<Posts />
<Photos />
</Suspense>
);

await act(async () => {
suspendedChunk.resolve({value, done: false, next: donePromise.promise});
donePromise.resolve({value, done: true});
});

expect(container.innerHTML).toBe('<p>loading posts and photos</p>');

await act(async () => {
await resolvePostsData('posts');
await resolvePhotosData('photos');
});

expect(container.innerHTML).toBe('<div>posts</div><div>photos</div>');
expect(reportedErrors).toEqual([]);
});

it('should preserve state of client components on refetch', async () => {
// Client

Expand Down