Skip to content

Commit

Permalink
Batch sync, default and continuous lanes (#25700)
Browse files Browse the repository at this point in the history 
<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn debug-test --watch TestName`, open
`chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
This is the other approach for unifying default and sync lane
#25524.
The approach in that PR is to merge default and continuous lane into the
sync lane, and use a new field to track the priority. But there are a
couple places that field will be needed, and it is difficult to
correctly reset the field when there is no sync lane.

In this PR we take the other approach that doesn't remove any lane, but
batch them to get the behavior we want.


## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->
yarn test

Co-authored-by: Andrew Clark <hi@andrewclark.io>
  • Loading branch information
@tyao1 @acdlite
tyao1 and acdlite authored Jan 5, 2023
1 parent bbf4d22 commit 5379b61
Show file tree
Hide file tree
Showing 26 changed files with 414 additions and 267 deletions.
27 changes: 21 additions & 6 deletions packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,17 +275,32 @@ describe('ReactDOMFiberAsync', () => {
expect(ops).toEqual([]);
});
// Only the active updates have flushed
expect(container.textContent).toEqual('BC');
expect(ops).toEqual(['BC']);
if (gate(flags => flags.enableUnifiedSyncLane)) {
expect(container.textContent).toEqual('ABC');
expect(ops).toEqual(['ABC']);
} else {
expect(container.textContent).toEqual('BC');
expect(ops).toEqual(['BC']);
}

instance.push('D');
expect(container.textContent).toEqual('BC');
expect(ops).toEqual(['BC']);
if (gate(flags => flags.enableUnifiedSyncLane)) {
instance.push('D');
expect(container.textContent).toEqual('ABC');
expect(ops).toEqual(['ABC']);
} else {
instance.push('D');
expect(container.textContent).toEqual('BC');
expect(ops).toEqual(['BC']);
}

// Flush the async updates
Scheduler.unstable_flushAll();
expect(container.textContent).toEqual('ABCD');
expect(ops).toEqual(['BC', 'ABCD']);
if (gate(flags => flags.enableUnifiedSyncLane)) {
expect(ops).toEqual(['ABC', 'ABCD']);
} else {
expect(ops).toEqual(['BC', 'ABCD']);
}
});

// @gate www
Expand Down
120 changes: 56 additions & 64 deletions packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -449,10 +449,9 @@ describe('ReactDOMServerPartialHydration', () => {
expect(deleted.length).toBe(0);

// Performing an update should force it to delete the boundary
root.render(<App value={true} />);

Scheduler.unstable_flushAll();
jest.runAllTimers();
await act(async () => {
root.render(<App value={true} />);
});

expect(hydrated.length).toBe(1);
expect(deleted.length).toBe(1);
Expand Down Expand Up @@ -945,13 +944,12 @@ describe('ReactDOMServerPartialHydration', () => {
root.render(<App text="Hi" className="hi" />);

// At the same time, resolving the promise so that rendering can complete.
suspend = false;
resolve();
await promise;

// This should first complete the hydration and then flush the update onto the hydrated state.
Scheduler.unstable_flushAll();
jest.runAllTimers();
await act(async () => {
suspend = false;
resolve();
await promise;
});

// The new span should be the same since we should have successfully hydrated
// before changing it.
Expand Down Expand Up @@ -1093,9 +1091,9 @@ describe('ReactDOMServerPartialHydration', () => {
expect(ref.current).toBe(null);

// Render an update, but leave it still suspended.
root.render(<App text="Hi" className="hi" />);
Scheduler.unstable_flushAll();
jest.runAllTimers();
await act(async () => {
root.render(<App text="Hi" className="hi" />);
});

// Flushing now should delete the existing content and show the fallback.

Expand All @@ -1104,12 +1102,11 @@ describe('ReactDOMServerPartialHydration', () => {
expect(container.textContent).toBe('Loading...');

// Unsuspending shows the content.
suspend = false;
resolve();
await promise;

Scheduler.unstable_flushAll();
jest.runAllTimers();
await act(async () => {
suspend = false;
resolve();
await promise;
});

const span = container.getElementsByTagName('span')[0];
expect(span.textContent).toBe('Hi');
Expand Down Expand Up @@ -1174,23 +1171,21 @@ describe('ReactDOMServerPartialHydration', () => {
expect(ref.current).toBe(span);

// Render an update, but leave it still suspended.
root.render(<App text="Hi" className="hi" />);

// Flushing now should delete the existing content and show the fallback.
Scheduler.unstable_flushAll();
jest.runAllTimers();
await act(async () => {
root.render(<App text="Hi" className="hi" />);
});

expect(container.getElementsByTagName('span').length).toBe(1);
expect(ref.current).toBe(span);
expect(container.textContent).toBe('');

// Unsuspending shows the content.
suspend = false;
resolve();
await promise;

Scheduler.unstable_flushAll();
jest.runAllTimers();
await act(async () => {
suspend = false;
resolve();
await promise;
});

expect(span.textContent).toBe('Hi');
expect(span.className).toBe('hi');
Expand Down Expand Up @@ -1252,20 +1247,21 @@ describe('ReactDOMServerPartialHydration', () => {
expect(ref.current).toBe(null);

// Render an update, but leave it still suspended.
root.render(<App text="Hi" className="hi" />);

// Flushing now should delete the existing content and show the fallback.
Scheduler.unstable_flushAll();
jest.runAllTimers();
await act(async () => {
root.render(<App text="Hi" className="hi" />);
});

expect(container.getElementsByTagName('span').length).toBe(0);
expect(ref.current).toBe(null);
expect(container.textContent).toBe('Loading...');

// Unsuspending shows the content.
suspend = false;
resolve();
await promise;
await act(async () => {
suspend = false;
resolve();
await promise;
});

Scheduler.unstable_flushAll();
jest.runAllTimers();
Expand Down Expand Up @@ -1490,13 +1486,12 @@ describe('ReactDOMServerPartialHydration', () => {
);

// At the same time, resolving the promise so that rendering can complete.
suspend = false;
resolve();
await promise;

// This should first complete the hydration and then flush the update onto the hydrated state.
Scheduler.unstable_flushAll();
jest.runAllTimers();
await act(async () => {
suspend = false;
resolve();
await promise;
});

// Since this should have been hydrated, this should still be the same span.
const newSpan = container.getElementsByTagName('span')[0];
Expand Down Expand Up @@ -1569,27 +1564,25 @@ describe('ReactDOMServerPartialHydration', () => {
expect(ref.current).toBe(null);

// Render an update, but leave it still suspended.
root.render(
<Context.Provider value={{text: 'Hi', className: 'hi'}}>
<App />
</Context.Provider>,
);

// Flushing now should delete the existing content and show the fallback.
Scheduler.unstable_flushAll();
jest.runAllTimers();
await act(async () => {
root.render(
<Context.Provider value={{text: 'Hi', className: 'hi'}}>
<App />
</Context.Provider>,
);
});

expect(container.getElementsByTagName('span').length).toBe(0);
expect(ref.current).toBe(null);
expect(container.textContent).toBe('Loading...');

// Unsuspending shows the content.
suspend = false;
resolve();
await promise;

Scheduler.unstable_flushAll();
jest.runAllTimers();
await act(async () => {
suspend = false;
resolve();
await promise;
});

const span = container.getElementsByTagName('span')[0];
expect(span.textContent).toBe('Hi');
Expand Down Expand Up @@ -2320,16 +2313,15 @@ describe('ReactDOMServerPartialHydration', () => {

// Render an update, which will be higher or the same priority as pinging the hydration.
// The new update doesn't suspend.
root.render(
<ClassName.Provider value={'hi'}>
<App text="Hi" />
</ClassName.Provider>,
);

// Since we're still suspended on the original data, we can't hydrate.
// This will force all expiration times to flush.
Scheduler.unstable_flushAll();
jest.runAllTimers();
await act(async () => {
root.render(
<ClassName.Provider value={'hi'}>
<App text="Hi" />
</ClassName.Provider>,
);
});

// This will now be a new span because we weren't able to hydrate before
const newSpan = container.getElementsByTagName('span')[0];
Expand Down
26 changes: 15 additions & 11 deletions packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1786,7 +1786,7 @@ describe('ReactDOMServerSelectiveHydration', () => {
document.body.removeChild(container);
});

it('can force hydration in response to sync update', () => {
it('can force hydration in response to sync update', async () => {
function Child({text}) {
Scheduler.unstable_yieldValue(`Child ${text}`);
return <span ref={ref => (spanRef = ref)}>{text}</span>;
Expand All @@ -1812,15 +1812,17 @@ describe('ReactDOMServerSelectiveHydration', () => {
const root = ReactDOMClient.hydrateRoot(container, <App text="A" />);
expect(Scheduler).toFlushUntilNextPaint(['App A']);

ReactDOM.flushSync(() => {
root.render(<App text="B" />);
await act(async () => {
ReactDOM.flushSync(() => {
root.render(<App text="B" />);
});
});
expect(Scheduler).toHaveYielded(['App B', 'Child A', 'App B', 'Child B']);
expect(initialSpan).toBe(spanRef);
});

// @gate experimental || www
it('can force hydration in response to continuous update', () => {
it('can force hydration in response to continuous update', async () => {
function Child({text}) {
Scheduler.unstable_yieldValue(`Child ${text}`);
return <span ref={ref => (spanRef = ref)}>{text}</span>;
Expand All @@ -1846,14 +1848,17 @@ describe('ReactDOMServerSelectiveHydration', () => {
const root = ReactDOMClient.hydrateRoot(container, <App text="A" />);
expect(Scheduler).toFlushUntilNextPaint(['App A']);

TODO_scheduleContinuousSchedulerTask(() => {
root.render(<App text="B" />);
await act(async () => {
TODO_scheduleContinuousSchedulerTask(() => {
root.render(<App text="B" />);
});
});
expect(Scheduler).toFlushAndYield(['App B', 'Child A', 'App B', 'Child B']);

expect(Scheduler).toHaveYielded(['App B', 'Child A', 'App B', 'Child B']);
expect(initialSpan).toBe(spanRef);
});

it('can force hydration in response to default update', () => {
it('can force hydration in response to default update', async () => {
function Child({text}) {
Scheduler.unstable_yieldValue(`Child ${text}`);
return <span ref={ref => (spanRef = ref)}>{text}</span>;
Expand All @@ -1878,11 +1883,10 @@ describe('ReactDOMServerSelectiveHydration', () => {
const initialSpan = container.getElementsByTagName('span')[0];
const root = ReactDOMClient.hydrateRoot(container, <App text="A" />);
expect(Scheduler).toFlushUntilNextPaint(['App A']);

ReactDOM.unstable_batchedUpdates(() => {
await act(async () => {
root.render(<App text="B" />);
});
expect(Scheduler).toFlushAndYield(['App B', 'Child A', 'App B', 'Child B']);
expect(Scheduler).toHaveYielded(['App B', 'Child A', 'App B', 'Child B']);
expect(initialSpan).toBe(spanRef);
});

Expand Down
Loading

0 comments on commit 5379b61

Please sign in to comment.