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

[core] Update tooling to run with React 18 #4155

Merged
merged 10 commits into from
Aug 4, 2022
Merged

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Mar 10, 2022

This PR updates the React version in all package.json to v18.x. As consequence, I had to update a bunch of tests to work in this version without warnings. I noticed 3 types of change it caused:

  • API calls now need to be wrapped in act to avoid warnings like

    Warning: An update to ForwardRef(DataGridPro) inside a test was not wrapped in act(...)

    Basically we only need to do:

    -apiRef.current.setFilterModel(...);
    +act(() => apiRef.current.setFilterModel(...));

    But it can get complex if the update happens inside a Promise so https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning may help.

  • When doing expect().toErrorDev([...]) we need to duplicate the first message.

    Now the errors generated by StrictMode are also sent to console.error. In React 17, the 2nd render made by StrictMode replaced console.error with a no-op version.

     expect(() => {
       render(
         <ErrorBoundary>
           <GridOverlay />
         </ErrorBoundary>,
       );
     }).toErrorDev([
       'MUI: useGridRootProps should only be used inside the DataGrid, DataGridPro or DataGridPremium component.',
    +  'MUI: useGridRootProps should only be used inside the DataGrid, DataGridPro or DataGridPremium component.',
       'The above error occurred in the <ForwardRef(GridOverlay)> component',
     ]);
  • When asserting that something was called X times, and one of the times was caused by an effect, we need to account for StrictEffects

    Each effect runs twice so the expectation has to consider that.

@mui-bot
Copy link

mui-bot commented Mar 10, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 246.4 500.4 329.6 342.98 92.151
Sort 100k rows ms 492 912.4 806.7 747.98 161.107
Select 100k rows ms 206.9 315.5 261.6 258.36 44.913
Deselect 100k rows ms 151.3 431.1 227.9 273.26 102.346

Generated by 🚫 dangerJS against 51e5822

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Mar 15, 2022
@m4theushw m4theushw self-assigned this Mar 23, 2022
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Mar 23, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 6, 2022
@shelimov

This comment was marked as off-topic.

@m4theushw

This comment was marked as off-topic.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jul 14, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 21, 2022
@m4theushw m4theushw changed the title [core] Support for React 18 [core] Update tooling to run with React 18 Jul 21, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 21, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 21, 2022
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jul 22, 2022
@mui mui deleted a comment from github-actions bot Jul 22, 2022
@mui mui deleted a comment from github-actions bot Jul 22, 2022
@mui mui deleted a comment from github-actions bot Jul 22, 2022
@mui mui deleted a comment from github-actions bot Jul 22, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 28, 2022
@github-actions

This comment was marked as outdated.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 1, 2022
Comment on lines 202 to 205
clock.tick(500);
fireEvent.keyDown(input, { key: 'Enter' });
await act(() => Promise.resolve());

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why some lines bellow you use

await act(() => clock.tick(500))

and here you can simply keep

clock.tick(500)

About await act(() => Promise.resolve()); would it make sense to create an helper

waitForTimeoutExecution = async () => act(() => Promise.resolve());

such that when reading the code await waitForTimeoutExecution() bring more information

Copy link
Member Author

Choose a reason for hiding this comment

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

Which line you're referring to? Note that the tests for the old editing API have some useless code, like waitFor being used with fake timers = no difference.

About the helper, we could add it but only waiting for the promise is not enough for "timeout completion". If the clock is fake, the time has to be manually advanced too.

Copy link
Member

Choose a reason for hiding this comment

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

Was looking at lin 240 await act(async () => clock.tick(500));

Copy link
Member Author

Choose a reason for hiding this comment

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

I reviewed this entire file removing useless code. Now most tests use only clock.tick(500) or clock.tick(500) + await act(() => Promise.resolve());.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 3, 2022
@github-actions

This comment was marked as outdated.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 3, 2022

await new Promise((resolve) => nativeSetTimeout(resolve)); // Wait for promise
await Promise.resolve(); // Wait for promise
Copy link
Member

Choose a reason for hiding this comment

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

Should this be also wrapped in an act? There is one more case couple of lines below.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the second one. It was doing nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants