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] Update matchMedia mocks #43240

Merged
merged 6 commits into from
Aug 13, 2024

Conversation

cherniavskii
Copy link
Member

The usage of deprecated addListener/removeListener was removed from useMediaQuery in https://github.com/mui/material-ui/pull/42464/files#diff-29dc32373d28b87f1bf18f66cfb725f9dbad15b86a308c8453aa0ce38d15c748L92
describeConformance still mocks the deprecated methods, which makes the tests in MUI X fail when running against @mui/material@^6:

TypeError: mediaQueryList.addEventListener is not a function

This PR updates the matchMedia mocks to use addEventListener/removeEventListener.

@cherniavskii cherniavskii added test scope: code-infra Specific to the core-infra product labels Aug 9, 2024
@mui-bot
Copy link

mui-bot commented Aug 9, 2024

@cherniavskii cherniavskii requested a review from a team August 9, 2024 11:07
addListener: () => {},
removeListener: () => {},
addEventListener: () => {},
removeEventListener: () => {},
Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, it should mock both here to work with both v5 and v6.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I follow. This PR is supposed to land on next (v6). How does keeping addListener/removeListener help v5?

Copy link
Member

Choose a reason for hiding this comment

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

Given the current release strategy for the internal packages, this should probably be a major release of the @mui/internal-test-utils and dropping the addListener altogether as the linked PR for removing Safari support did. 🤔
What do others think?
cc @mui/code-infra

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is supposed to land on next (v6). How does keeping addListener/removeListener help v5?

We're using the @mui/internal-test-utils released from the next branch, but we can run our tests using either @mui/material v5 or v6. Since @mui/material@5 still uses addListener/removeListener, we need both to run the tests against both v5 and v6.
Does this make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Got it 👍

Copy link
Member

Choose a reason for hiding this comment

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

Given the current release strategy for the internal packages, this should probably be a major release of the @mui/internal-test-utils

We're not following semver with internal packages. The strategy is to be always backwards compatible (*). So I believe supporting both as @cherniavskii does makes most sense to me 👍

(*) backwards compatible against our own code that is. As soon as none of our code uses the compatibility mode anymore, it can be dropped, even on a minor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good 👍🏻
This PR is ready for review then.

cherniavskii added a commit to cherniavskii/mui-x that referenced this pull request Aug 9, 2024
@cherniavskii cherniavskii marked this pull request as ready for review August 9, 2024 12:44
cherniavskii added a commit to cherniavskii/mui-x that referenced this pull request Aug 9, 2024
@cherniavskii
Copy link
Member Author

Is the failed "CI / test-dev (macos-latest) (pull_request)" step a false alarm? I'm not sure my changes could have caused it 🤔

@Janpot
Copy link
Member

Janpot commented Aug 13, 2024

Can't reproduce locally (pnpm --filter @app/next-app build). I'll rerun the job, just to make sure. Looks like one of those non-deterministic vite/Next.js getting confused over our strange ESM implementation. I've restarted the test just to make sure.

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.

Suggestion: It could be valuable to have a comment that explains why the legacy methods are still there and when they can be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: code-infra Specific to the core-infra product test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants