Skip to content
This repository has been archived by the owner on Nov 20, 2021. It is now read-only.

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

Merged
merged 1 commit into from
Feb 11, 2017

Conversation

vadimdemedes
Copy link
Contributor

@vadimdemedes vadimdemedes commented Feb 10, 2017

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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 fregante merged commit 837628f into fregante:master Feb 11, 2017
@fregante
Copy link
Owner

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
Copy link
Owner

2.0.2 is out! Thanks :)

@vadimdemedes
Copy link
Contributor Author

Great, thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants