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

fix: support require()-ing video.js #3889

Merged
merged 1 commit into from
Dec 23, 2016

Conversation

misteroneill
Copy link
Member

@misteroneill misteroneill commented Dec 23, 2016

Description

This makes a number of small fixes to support pulling in video.js via require() in Node.

I have verified it in the following ways (using Node 4.6):

node -e "require('./dist/video.js')"
node -e "require('./es5/video.js')"
node
> require('./dist/video.js')
node
> require('./es5/video.js')

All these were tested because we had noticed some odd behavior when using the inline -e command vs. the REPL. Success was considered "didn't throw an error".

Note: It is not expected that calling videojs() the function in Node will work. That will throw an error, but loading it as a module should not throw.

Fixes #3869.

Specific Changes proposed

  • Introduce the Dom.isReal() function, which makes an educated assumption about the "realness" of the document object.
  • Wrap code here and there with checks against Dom.isReal() as well as other defensive code.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors

@misteroneill misteroneill added the patch This PR can be added to a patch release. label Dec 23, 2016
@@ -89,10 +96,10 @@ function autoSetupTimeout(wait, vjs) {
videojs = vjs;
}

setTimeout(autoSetup, wait);
window.setTimeout(autoSetup, wait);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setTimeout is a global in node too, so this change isn't really needed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but it's consistent with the rest of the codebase where we are using window.setTimeout, which will work in Node because we're using the global package.

* @return {Boolean}
*/
export function isReal() {
return document === window.document && typeof document.createElement === 'function';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would need a typeof window !== 'undefined' and typeof document !== 'undefined', since otherwise they're a reference error in node.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we're using global, it returns either window or global depending on the environment, so, window should never end up being undefined.

@gkatsev gkatsev merged commit ac0b03f into videojs:master Dec 23, 2016
@misteroneill misteroneill deleted the support-node branch December 23, 2016 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow video.js to be run in node
3 participants