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

[SSR] Values are ignored when window is present #161

Closed
michalpleszczynski opened this issue Jun 28, 2018 · 9 comments
Closed

[SSR] Values are ignored when window is present #161

michalpleszczynski opened this issue Jun 28, 2018 · 9 comments

Comments

@michalpleszczynski
Copy link
Contributor

For SSR it might be needed to detect the initial client load and make sure it's going to render the same thing as on the server (see relevant discussion here)

This is problematic with MediaQuery component, because even if values props is specified it is ignored if window is defined, instead window.matchMedia is always used. As far as I know currently there is no way to force MediaQuery to behave exactly as on server, so if the window width that is guessed on the server (in the absence of window) matches different MediaQuery components, in React 16+ we will end up with content mismatch errors.

I've forked both react-responsive and matchmediaquery to make this possible, I'm not sure if anybody here would like to introduce a change like this, so I'm leaving it here for your consideration :)

@yocontra
Copy link
Owner

values should always take precedence, looking at the code this seems to be the assumption I made: https://github.com/contra/react-responsive/blob/master/src/index.js#L54-L64

But matchmediaquery doesn't operate that way, so I think the best course of action would be to send your changes as PRs and we can merge it in. This would explain another issue where in phantom tests they weren't able to force the values to work.

@yocontra
Copy link
Owner

Nice find, btw - not sure how that slipped through. Tests would be great for this if you have time to include some in your PR.

@michalpleszczynski
Copy link
Contributor Author

@contra I've created a PR for matchmediaquery, not sure if this repo is actively developed though. Let's see if it goes through and then I'll make a PR with tests here

@michalpleszczynski
Copy link
Contributor Author

Ok, my PR to matchmediaquery was merged. I've tried adding some meaningful tests here, but they would have to use the sinon stub and I'm not sure it's working as expected. @contra Could you look at index_test.js - after removing before and after (stub setup), all tests are still passing. Inspecting this.mmStub.getCalls() in tests always returns empty array.

@yocontra
Copy link
Owner

@michalpleszczynski Can you link me to the tests? Should be good to send the PR now and we can iterate on it there.

@michalpleszczynski
Copy link
Contributor Author

@contra here you go. I've added 3 tests, one is skipped right now, see TODO note

yocontra pushed a commit that referenced this issue Jul 15, 2018
@yocontra
Copy link
Owner

Published fix as 5.0.0 - thanks again!

@oyeanuj
Copy link

oyeanuj commented Jul 28, 2018

@michalpleszczynski @contra Wouldn't this be the problem since the values in the previous versions was a placeholder to render a default state, and with this change, it will render that even if the actual sizes on the client is different?

I feel like overriding prop, and a default prop are two different concepts that are being merged here?

@yocontra
Copy link
Owner

yocontra commented Jul 28, 2018

@oyeanuj The intended and documented behavior was always that values would provide the "environment" if no browser was present, or for testing in a browser. Testing was the original reason it was made, SSR was a happy side-effect IIRC. Using values for SSR was always a hack, and in react 16 quit working correctly - that and other SSR stuff is being discussed in #162

Since this unintended behavior was being relied on this change was published as a breaking change, so people with existing code can continue to use the 4.x branch without issues.

(If i didn't fully answer your question LMK - I think I understood what you were asking)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants