Skip to content

Conversation

@LukasTy
Copy link
Member

@LukasTy LukasTy commented Aug 15, 2024

userEvent.mousePress or userEvent.keyPress do not seem to be used in this repo.
mui-x is using those extensively and never in an environment where React < 18.

The existing function has an inconsistent return type (act returns a Promise).

Also renamed userEvent export to fireUserEvent to avoid the confusion with @testing/library/user-event and in turn errors produced by the eslint-plugin-testing-library when userEvent calls are detected (plugin assumes they are from @testing-library/user-event and complains if they are not awaited).

Based on #43313 (comment) decided to remove the userEvent export.

@LukasTy LukasTy added test scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). labels Aug 15, 2024
@LukasTy LukasTy self-assigned this Aug 15, 2024
@mui-bot
Copy link

mui-bot commented Aug 15, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against de30a2a

@LukasTy LukasTy added the breaking change Introduces changes that are not backward compatible. label Aug 16, 2024
@LukasTy LukasTy marked this pull request as ready for review August 16, 2024 10:40
@LukasTy LukasTy requested a review from a team August 16, 2024 10:40
@Janpot
Copy link
Member

Janpot commented Aug 16, 2024

Are they still needed given that we also use @testing-library/user-event? It already has APIs for mouse and keyboard on board.

@LukasTy
Copy link
Member Author

LukasTy commented Aug 16, 2024

Are they still needed given that we also use @testing-library/user-event? It already has APIs for mouse and keyboard on board.

I've explored the option of using solely the @testing-library/user-event instead of these "fakes", but it created timeout issues in tests. 🙈
I think that such refactoring is better done separately from the ESLint plugin introduction. 👍

@Janpot
Copy link
Member

Janpot commented Aug 16, 2024

The existing function has an inconsistent return type (act returns a Promise).

🤔 doesn't it only return a promise when called with an async function? In any case, I'd rather make those methods async and keep the act, but just calling it asynchronously.

@LukasTy
Copy link
Member Author

LukasTy commented Aug 16, 2024

doesn't it only return a promise when called with an async function? In any case, I'd rather make those methods async and keep the act, but just calling it asynchronously.

act in those functions returns a Promise when called in an environment with React < 18.
But AFAIK, there are no instances where tests using these functions are run on older React versions. 🤷
If you don't agree with the change, I can only keep the rename of the export, which is mainly needed for the Eslint plugin setup on mui-x. 🤔

@Janpot
Copy link
Member

Janpot commented Aug 16, 2024

Would it make sense to just move these functions to X and remove them here? I don't see why we would use them over @testing-library/user-event.

@LukasTy
Copy link
Member Author

LukasTy commented Aug 16, 2024

Would it make sense to just move these functions to X and remove them here? I don't see why we would use them over @testing-library/user-event.

Great observation. 💯
I will look into that.

@LukasTy LukasTy changed the title [code-infra] Drop support for React<18 in @mui/internal-test-utils/userEvent [code-infra] Remove userEvent export from @mui/internal-test-utils Aug 16, 2024
@LukasTy LukasTy merged commit 14b615e into mui:next Aug 16, 2024
@LukasTy LukasTy deleted the remove-older-react-support-from-test-utils branch August 16, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Introduces changes that are not backward compatible. scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants