fregante / iphone-inline-video

📱 Make videos playable inline on the iPhone (prevents automatic fullscreen)
https://npm.im/iphone-inline-video
MIT License
2.05k stars 300 forks source link

Allow loading in Node.js without setting up jsdom #105

Closed vadimdemedes closed 7 years ago

vadimdemedes commented 7 years ago

Hey, thanks for the great lib, very useful!

This PR adds a tiny fix when checking for iOS 8 or 9. By checking if document exists at all, it allows loading iphone-inline-video in tests (Node.js) without setting up jsdom.

Avoids this:

var isWhitelisted = 'object-fit' in document.head.style && /iPhone|iPod/i.test(navigator.userAgent) && !matchMedia('(-webkit-video-playable-inline)').matches;
                                    ^

ReferenceError: document is not defined
vadimdemedes commented 7 years ago

In case of AVA tests, iphone-inline-video can't be loaded at all, because AVA requires dependencies upfront, before executing test file. As a result, we don't have a chance to even set up jsdom before iphone-inline-video is loaded.

fregante commented 7 years ago

It can be done. I already had some Ava tests and this is how I was applying JSdom https://github.com/bfred-it/iphone-inline-video/blob/tests-test/test/helpers/setup-browser-env.js

vadimdemedes commented 7 years ago

Yes, but it's more of a workaround than a a fix for the underlying issue. This change isn't breaking, I think, so there shouldn't be any surprises.

fregante commented 7 years ago

Have you tested this change? I don't think it's enough to make it work in node.

I'm not entirely convinced that this code belongs to the client library itself either. Are you going to send PRs to each and every client-only library that uses the DOM?

vadimdemedes commented 7 years ago

Have you tested this change? I don't think it's enough to make it work in node.

Yes, I did.

Are you going to send PRs to each and every client-only library that uses the DOM?

No, only the ones I use and highly depend on.

fregante commented 7 years ago

Cool, for a moment I thought it would fail on matchMedia as well.

Ok the change is small enough. I'd love to know if there are alternative solutions eventually (i.e. have a client-only build)

fregante commented 7 years ago

2.0.2 is out! Thanks :)

vadimdemedes commented 7 years ago

Great, thanks!