provide optional targetWindow prop

[raycohen]

We didn't realize that master had changed since the 1.6.1 release when we started this work.

We wrote tests but haven't tested this against live code because we ran into issues yarn linking it to our project. Our create-react-app was complaining about the syntax in theesm build artifact.

Wanted to share and get your feedback. Maybe you know why we had issues with using the build?

paired with @tonyjmnz

[mjackson]

I'd prefer to not have a targetWindow prop if we can avoid it. Feels like an option for something we should be able to determine automatically.

[raycohen]

@mjackson the only way I can think of to auto-detect the iframe would be to wait until after the Media component mounts, use a ref callback to get an element, and use element.ownerDocument.defaultView to get the window.

There are 2 problems with this - first, Media doesn't have its own DOM elements so I'm not sure where to put a ref. Second problem is that if we wait until Media mounts to know what window to use matchMedia with, we would have to render blank content initially or risk a flash where the wrong content might show up.

Do you have thoughts on that or other avenues we can explore?

[mjackson]

What about window.top === window?

[tonyjmnz]

The problem we initially saw with that approach was that with our current iframe-rendering implementation (using react-frame-component at the moment) the Media component would get rendered in the parent window's context and then passed down to the iframe, so the window.top === window comparison would always be true. We'll give it another shot to see if we can avoid adding that extra prop.

[mjackson]

Huh, interesting. Do you know how stuff gets passed to the iframe? Are you rendering a string and passing in HTML?

[tonyjmnz]

The library we're using is using ReactDOM.unstable_renderSubtreeIntoContainer, a pre-react16 alternative for portal-like behavior https://github.com/ryanseddon/react-frame-component/blob/master/src/Frame.jsx#L124

[mjackson]

AFAIK the only reason you need to use unstable_renderSubtreeIntoContainer is if you still need things like context to work. So you might not need it.

In any case, it sounds like you (and probably others) may end up needing the targetWindow prop eventually. And it has a sensible default, so it shouldn't be a big deal to add it. However, instead of adding it in defaultProps as the PR currently does, it'd be better to just default to using it in componentDidMount so people can still render a <Media> server-side w/out requiring a DOM environment.

[mjackson]

This still breaks server-side rendering because componentWillMount fires on the server but window is not defined.

[mjackson]

This is the line that checks to make sure we're actually in a browser. It should stay:

if (typeof window !== 'object') return;

and then get the targetWindow prop afterwards.

[mjackson]

Looks good. Thanks, guys!

[mjackson]

This was released in 1.8.0-rc.1. Please give it a shot and let me know if it works for you!

npm install react-media@next

[tonyjmnz]

worked like a charm, thanks for merging!