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

Regression: withWidth fails on the server side because of useMediaQuery #16184

Closed
2 tasks done
fzaninotto opened this issue Jun 12, 2019 · 9 comments · Fixed by #16196
Closed
2 tasks done

Regression: withWidth fails on the server side because of useMediaQuery #16184

fzaninotto opened this issue Jun 12, 2019 · 9 comments · Fixed by #16196
Assignees
Labels

Comments

@fzaninotto
Copy link
Contributor

Since #15678, the withWidth() HOC fails on the server-side (in unit tests run with react-testing-library) with the following error:

TypeError: window.matchMedia is not a function

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

No regression in a bugfix release, unchanged withWidth() behavior on the server side since 4.0.0

Current Behavior 😯

Because it now uses the useMediaQuery hook, withWidth() fails on the server side.

I know I could ask ssr: false somehow but I prefer not to deal with that (just like withWidth used to work)

Steps to Reproduce 🕹

Sorry, I can't provide a link to a CodeSandbox as it only happens in the server side.

Just render a component decorated with withWidth() using react-testing-library to get the error.

Context 🔦

I'm migrating react-admin to material-ui v4 and its hooks.

Your Environment 🌎

Tech Version
Material-UI v4.0.0-beta2
React 16.8.0
Browser Chrome
TypeScript No
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 12, 2019

@fzaninotto If you are mounting the components, you need the browser API to be available. It seems that JSDOM doesn't implement it yet: https://github.com/jsdom/jsdom/blob/94774014e2244730f24d1fcb8c40d8fee3ea6b75/test/web-platform-tests/to-upstream/html/browsers/the-window-object/window-properties-dont-upstream.html#L105.

I'm wondering what we should do about it.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 12, 2019

I would propose to add a defensive check in the core, it's the least worse solution I can think of. Would it work for you?

@fzaninotto
Copy link
Contributor Author

If you are mounting the components, you need the browser API to be available

I didn't need it with the 4.0.0 implementation of withWidth. Why add such a requirement in a bugfix release? This is a BC break.

@fzaninotto
Copy link
Contributor Author

I would propose to add a defensive check in the core, it's the least worse solution I can think of. Would it work for you?

As long as it behaves like the previous implementation, all right for me!

@oliviertassinari
Copy link
Member

Ok, I think that we can move forward with that. Thanks for raising.

@oliviertassinari oliviertassinari modified the milestones: v4.1, v4.x Jun 12, 2019
@fzaninotto
Copy link
Contributor Author

fzaninotto commented Jun 12, 2019

If I can add just one more request: can you add the defensive check at the useMediaquery level, so that the migration from withWidth is natural?

@oliviertassinari
Copy link
Member

💯

@MrXyfir
Copy link

MrXyfir commented Jun 12, 2019

If you're using Jest and want to mock it, the following works (at least for my use case):

window.matchMedia = jest.fn(() => ({
  matches: true,
  media: '',
  addListener: jest.fn(),
  removeListener: jest.fn()
}));

I don't know how this changes things internally, but seems fine for a temporary hack to get tests passing...

@fzaninotto
Copy link
Contributor Author

Thanks for the super fast fix!

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 a pull request may close this issue.

3 participants