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

[test] Add missing async #44028

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Oct 6, 2024

Same as mui/base-ui#706

@mui-bot
Copy link

mui-bot commented Oct 6, 2024

Netlify deploy preview

https://deploy-preview-44028--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 643ad01

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Oct 6, 2024

It looks correct actually mui/base-ui#706, the others are wrong.

@oliviertassinari oliviertassinari changed the title [test] Remove redundant async [test] Add missing async Oct 6, 2024
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Tried an eslint rule but I noticed our codebase is full of sync act

'no-restricted-syntax': [
  'error',
  {
    selector: "CallExpression[callee.name='act'] > ArrowFunctionExpression:not([async=true])",
    message: 'Synchronous form of `act` is not allowed. Use `await act(async () => {...})` instead.',
  },
  {
    selector: "CallExpression[callee.name='act']:not(AwaitExpression > CallExpression[callee.name='act'])",
    message: '`act` must be awaited. Use `await act(async () => {...})` instead.',
  },
],

We could plan some time to do a full sweep of the codebase, in my limited experience the sync version doesn't always work as expected. I wouldn't be surprised if mui/mui-x#14668 magically starts working if we remove all sync act calls.


Off-topic, but copilot is really helpful when you want to write those rules

Craft an eslint no-restricted-syntax rule that disallows using act in its synchronous form. It disallows act(() =>{ ... and only allows act(async () => { .... Likewise, add a rule that prevents calling act without being awaited on.

Flawless, in seconds, no digging through AST-explorer and trying to reverse engineer their selector syntax

@@ -260,7 +260,7 @@ describe('<Menu />', () => {
const item1 = getByTestId('item-1');
const item2 = getByTestId('item-2');

await act(() => {
await act(async () => {
Copy link

@potapovDim potapovDim Oct 7, 2024

Choose a reason for hiding this comment

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

why async is needed here if
items[0].focus(); does not have await ?
it make sense to have it as
await act(async () => items[0].focus());
or

await act(async () => {
    await items[0].focus();
})

Copy link
Member

@Janpot Janpot Oct 7, 2024

Choose a reason for hiding this comment

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

From https://react.dev/reference/react/act#await-act-async-actfn

We recommend using act with await and an async function. Although the sync version works in many cases, it doesn’t work in all cases and due to the way React schedules updates internally, it’s difficult to predict when you can use the sync version.

We will deprecate and remove the sync version in the future.

or, the async is there because it changes the behavior of act. I understand it looks a bit strange to not await anything. but it's still nicer than doing

await act(() => {
  items[0].focus();
  return Promise.resolve();
})

No need to await items[0].focus(). It doesn't return a promise.

@mnajdova mnajdova merged commit 7ff80b8 into mui:master Oct 8, 2024
19 checks passed
@oliviertassinari oliviertassinari deleted the test-redundant-async branch October 8, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants