Skip to content

Conversation

wlingke
Copy link

@wlingke wlingke commented Dec 5, 2017

#14

@wlingke
Copy link
Author

wlingke commented Dec 12, 2017

@JedWatson any thoughts on this?

@wlingke
Copy link
Author

wlingke commented Feb 1, 2018

@bradleyflood You've made a bunch of statements without any backing... I'm not sure why you think "Checking for only window is quite standard." I see no evidence behind that.

On the contrary, I've frequently seen that document is what's checked. window may be mocked for many reasons such as when using webpack's require.ensure. Obviously I have no stats for whether window + document or window or document are more commonly used, but here are at least a number of SSR blog examples that use document alone, without window. It's not the best evidence, but at least shows the community does in fact check for document

https://medium.com/front-end-hacking/preparing-react-components-for-server-side-rendering-18901d09784c
https://survivejs.com/webpack/output/server-side-rendering/
http://jxnblk.com/writing/posts/static-site-generation-with-react-and-webpack/
https://github.com/markdalgleish/static-site-generator-webpack-plugin

Also, not sure what this problem is: Another team may even be mocking document = {} on their server side and then they will be in the same scenario you are too.

Suppose you were expecting require('hammerjs') to not be called. This means you previously had typeof(window) === 'undefined'. Regardless of whether you mocked document, this still happens post-PR.

Now suppose you were expecting require('hammerjs') to be called. This means you previously mocked window. If you have also mocked document={}, then the require statement is still called.

So I'm not sure what the problem you're stating is.

The only scenario that has a breaking change is if they were expecting require('hammerjs') to be called, had mocked window and did NOT mock document. I'm not entirely sure this scenario is a frequent use case IMHO...

@wlingke
Copy link
Author

wlingke commented Mar 13, 2018

@JedWatson I submitted this PR over 3 months ago. Any chance you could spend 10 minutes to review and let me know if this is mergeable? Thanks!

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

Successfully merging this pull request may close these issues.

1 participant