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

Still broken in node #3536

Closed
ljharb opened this issue Aug 16, 2016 · 7 comments
Closed

Still broken in node #3536

ljharb opened this issue Aug 16, 2016 · 7 comments

Comments

@ljharb
Copy link
Contributor

ljharb commented Aug 16, 2016

Even with #3502 applied locally, I get:

$PWD/node_modules/video.js/dist/video.js:19742
var BACKGROUND_SIZE_SUPPORTED = ('backgroundSize' in _globalDocument2['default'].createElement('video').style);
                                                                                 ^

TypeError: _globalDocument2.default.createElement is not a function
    at Object.140.global/document ($PWD/node_modules/video.js/dist/video.js:19742:82)

After trying to patch that locally, I get:

$PWD/node_modules/video.js/dist/video.js:15920
  supportsTextTracks = !!Html5.TEST_VID.textTracks;
                                       ^

TypeError: Cannot read property 'textTracks' of undefined
    at Function.Html5.supportsNativeTextTracks ($PWD/node_modules/video.js/dist/video.js:15920:40)

After patching that, I get:

$PWD/node_modules/video.js/dist/video.js:15940
  var supportsVideoTracks = !!Html5.TEST_VID.videoTracks;
                                            ^

TypeError: Cannot read property 'videoTracks' of undefined
    at Function.Html5.supportsNativeVideoTracks ($PWD/node_modules/video.js/dist/video.js:15940:45)

Could the project add a test that requires dist/video.js, in node, so as to ensure none of these continue?

@gkatsev
Copy link
Member

gkatsev commented Aug 16, 2016

What specifically are you trying to with videojs in node?
I always find it weird that we get issues around createElement because we tend to use global which should return min-document for document in node and that is supposed to have a createElement method.
Also, the way that some of the internals of videojs work is that they expect to be run in a browser context and changing that is tricky and probably not worth the effort because they rely so much on things available in the browser.

@gkatsev
Copy link
Member

gkatsev commented Aug 16, 2016

Oh, I just tried requiring global directly and it works fine but has issues if it's done through the current dist file in videojs. It should be fixed by #3445.

@ljharb
Copy link
Contributor Author

ljharb commented Aug 16, 2016

Our react components run in both the browser and server, but we use componentDidMount to only invoke videojs on the server. I need to be able to require videojs in both environments, and only have browser things invoke on the browser. In other words, I need to be able to require any module without side effects in any environment.

It's fine if videojs doesn't actually work in node - it just needs to be requireable without error.

@gkatsev
Copy link
Member

gkatsev commented Aug 16, 2016

Ah, ok, making it requirable sounds fine to me.

@gkatsev
Copy link
Member

gkatsev commented Aug 22, 2016

Fixed by #3540.

@gkatsev gkatsev closed this as completed Aug 22, 2016
@ljharb
Copy link
Contributor Author

ljharb commented Aug 22, 2016

Thanks! ETA on an npm release?

@gkatsev
Copy link
Member

gkatsev commented Aug 22, 2016

I would like to get #3541 in before a release but definitely this week since I'll be going on vacation starting next week.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants