-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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: guard case when window is undefined in node environment #1642
Conversation
@@ -1,5 +1,5 @@ | |||
const requestMediaKeySystemAccess = (function () { | |||
if (window.navigator && window.navigator.requestMediaKeySystemAccess) | |||
if (typeof window !== 'undefined' && window.navigator && window.navigator.requestMediaKeySystemAccess) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why you can't just do if (window &&
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in Node, the window object does not exist. "window" has no reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm curious; there are several references to window
throughout the codebase without this guard. Is other code (window.TextTracks
, for example) also throwing exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am assuming those guards are in areas that are not in the execution path of simply loading the library (versus calling a function that evokes window.whatever).... We have this library bundled and running in an isomorphic application, but on the server nothing is ever called. It just needs to be bundled safely. But yes, all these checks could be done the same way :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok makes sense. I see other guard checks like this in other modules. In the future I think it would be better to solve this at a higher level but I won't hold up this PR for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! Thanks!
see #1642 >We have this library bundled and running in an isomorphic application, but on the server nothing is ever called. It just needs to be bundled safely.
@tjenkinson @johnBartos @patrickmichalina I don't really understand how this makes any sense really. If you want to require this in Node, you could mock up the window in Now that we would want to deal with globals in a safe way, this breaks because |
basically, it leaves us with having to use a wrapper around window in every possible place: 40bd49c
That was the right question to ask, and actually now we have the situation where |
The latest release is the issue, previous to 0.9.1, loading in Node was perfectly OK. This is the line that fails. |
Yes, sure. But the thing is that we are having a few bugs around global vars masked in our linter, so we made globals explict everywhere and that now breaks the node require :) |
@patrickmichalina no worries, we'll manage, it's just a bit of a hassle. I guess I'd just like to raise the question wether it is sane that we need to do this in the first place :) |
@patrickmichalina Also, instead of us having to take care of this for you (which is a tiny bit weird because we are a DOM dependent lib, and not a Node lib), you could just add DOM support to your Node runtime. There are many options. One is: https://github.com/jsdom/jsdom But also it could be as simple as saying On the other hand us having to support this, not even for actual functionnality but just because your build-system somehow runs that code in an environment it's not thought for, makes this major hassle for us, while it would be very easy to fix it on your side. Btw I think that the first very undescriptive line of this PR here is wrong: This is not a bug in the first place. |
Totally! I don't want it to break anything :) |
I agree it seems a bit weird to make sure we don't use 'window' on the initial path to work around how an external build tool works. This pr was a simple change though, but if it's going to make hlsjs more complex then I think we should rethink this. @patrickmichalina is it not possible in your application to have the require return a stub when running in node? |
@tjenkinson its possible, no doubt. IMO, I do think the library should be flexible enough to not require this stub to exists in the consuming app. |
i understand that somehow your build tool does an actual node-require which runs the module declaration - i don't quite understand why that is necessary to bundle the code for the frontend in any way, it doesn't seem to be a sane and robust thing to do imo, you will most probably run into many issues with other js libs that depend on dom globals upon their declaration. at least i don't think it should be a requirement for modules written for frontend to be able to handle that. i think you have in this case to change something in the design of the application, or allow your build tool to overload require in order to return something trivial for external frontend libs. but why does the code around hls.js need to be that much tightly coupled with code that runs in Node? Just being curious really :) anyhow, if you would want to use 0.9.1 "right now", there are many quick fixes that have been proposed here. I'd actually vote for reverting this here, as well as the CI check that will try to require the dist. If we want to do some sort of global-scope DI, we can think about the benefits, and then it could facilitate also these use-cases. But right now I don't see a motivation to put effort in this. |
Not sure why its "bad" to check if the window object exists. Considering the current state of javascript running in non-browser environments. I disagree that it is tightly coupled. I am simply importing the library for use in an isomorphic application. Still not understanding what the fuss is about. |
it's not that simple. for example you might think we can just put a check for stuff like Now, therefore what this forces us to do is having a util function like I hope you understand better my concern and point and view. Can you please explain what an isomorphic app is please? |
The application runs the majority of code in both server and clients (usually browser). For this app in particular we are using https://universal.angular.io. |
Oh I see, this is about SSR. Now I understand :) You are doing programmatic DOM building with Angular on the frontend, but then run that and render it to textual HTML on the server for SEO. Got that, I've seen it being done with React too, makes total sense. I guess you can just say though that you are server-side-rendering your Angular-App, and I guess people may understand bettter what you really mean, instead of isomorphic, which sounds like it could be many things, at least fmpov :) Now that you mention the tooling you use, seems this is actually an open issue around Angular Universal, that hasn't been quite figured out yet (since it is still an open issue labeld "window not defined"): angular/universal#830 There a lot of solutions there which have been proposed ^ For example using libs emulating the DOM methods etc, or adding a dummy global.window stub, as we had already suggested you should do. Also, I guess what I meant with tightly coupled is the following: It makes sense to server-side render things that can actually be server-side rendered. If you are telling your SSR to run on code using the player, it doesn't make any sense, since it is not possible to render that on the server. You should abstract away from that component which is using Hls.js when you do the SSR and replace it by some dummy container I guess, or whatever you would like search-bots to "see" at that place. If your SSR tool for Angular doesn't allow you to do that, I would claim it's not a good tool and it hasn't been thought til the end. Also, I guess Angular is much about re-using components and composing your app with them, so I guess the thing that requires Hls.js shouldn't have to be right at the main entry point of your app, right? And also, I would be surprised if the Angular SSR toolchain you use wouldn't have a goto solution for this already, you should maybe do some research further about this :) It took me 1 second to google "Angular Universal window not defined" to find the issue link above. As I said, if we had such a utility function, we could not justify it for your use-case imo, since this stuff has to be solved on your end. If we had such a method it would be to be able to inject an arbitrary window scope. But I don't see why we would want to do that. If I would want to run parts of Hls.js on Node, I would probably just set all the necessary things on |
This PR will...
Fix a bug
Why is this Pull Request needed?
Version 0.8 was working fine, but now with 0.9 code can no longer be loaded in node environment
Are there any points in the code the reviewer needs to double check?
Resolves issues:
allows node code to execute without failing on "window is undefined"
Checklist