-
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
Support isomorphic/SSR environments (Node.js) #1813
Comments
Are you not able to skip loading the library when running in node? It seems strange that a library that is meant to be used in the web browser also has to partly work in node. There is a discussion about this here: #1642 |
Strange, yes. Unfortunate, yes. Needed for isomorphic? Also yes. |
Erm, wait it's merged, has this hit a release? |
It was reverted because we decided it isn't really make sense and should be handled by the library you are using. It just adds extra overhead to hls.js which is not required for it to function in a browser. Please can you provide more information about your setup and the build system you are using? |
Angular CLI 6, latest version using the built-in SSR support. Well, at the
very least my description above provides a workaround.
…On Mon, Jul 2, 2018, 1:30 PM Tom Jenkinson ***@***.***> wrote:
It was reverted because we decided it isn't really make sense and should
be handled by the library you are using. It just adds extra overhead to
hls.js which is not required for it to function in a browser. Please can
you provide more information about your setup and the build system you are
using?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1813 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAY-NPAhfiGjC80DNIaJMAwqdOadHPe5ks5uCoLIgaJpZM4U_0mQ>
.
|
If you just want the You should be able to add this to all bundles using Webpack. I haven't tried, but adding it for Rollup was easy:
|
No the import isn't the problem, the unguarded use of window is, as I said. Node.js doesn't have a window object, so it's NameError on execution. I could polyfills I will eventually clean that up, polyfill window, and bundle hlsjs but for now I'm pulling it from cdn on demand which is a fine workaround for the time being. Edit: And conditional checks are something to avoid when your build tool is statically analyzing your code, though it might still work if someone hits this and is braver than me, but I've been burnt in subtle and nonobvious ways doing that in the past with Angular CLI's builds EDIT (way later): I simply misunderstood that the previous comment was targetted at an hls.js improvement as opposed to a workaround for users of hls.js |
I guess you're right, and all these people are wrong then: sampotts/plyr#935 ;) |
I believe those people agree with me actually? Look I'm not declaring any right or wrong here, just pointing out that hls.js doesn't play nicely with SSR as well as it could, but I understand that it might be more work than it's worth to make that happen, and SSR folks will have to work around that, whether by not bundling hls.js and using CDN instead or by a (potentially risky) bypass of the static analysis systems of their build tools. It's all good! |
That's the same PR I linked to and described in the first post which you violently disagreed with.
Really? How about "No the import isn't the problem, the unguarded use of window is, as I said" Addition: |
Oh I see, sorry you were offended by my tone, there was no malicious intention. It's just that these tools are ESM tools, and require() can really cause some headaches in my experience when the tools are speaking ESM. And you cannot conditionally Edit: yeah that PR is doing what this issue originally advocated for: fix the library to not assume environments, at least during load if not during execution. |
Closing this as it's fairly clear it's already been addressed elsewhere. |
Hls.js is built to UMD: So adding a "header" to the build such as my suggestion still seems feasible to me. And also wouldn't affect any static code analysis like you previously suggested unless you run them on the build output (which wouldn't be a great use for static code analysis). The fact that the source code is ESM doesn't matter unless you're specifically importing it from
I know of it, which is why again I wanted to help and linked to + explained a feasible solution (Plyr also uses ESM internally but the distribution builds to UMD).
As has been mentioned, the previous PR was reverted because it tackled the solution in the wrong way, and didn't fully solve the problem. That's why I posted. Edit: |
I don't think I'm the one being unreasonable about this. It sounds like what you want to do is change hls.js to make this work better, and that's great, I'm all for it. Edit: Feel free to refile, I'm unsubscribing on this. |
For isomorphic web apps which can be run on the server-side in Node.js, hls.js cannot be directly bundled into the application because of it's unguarded use of
window
and other browser-only APIs.Ideally hls.js should detect this condition and prevent usage of browser APIs when running on the server.
The workaround for isomorphic applications is to load it separately from CDN and then utilize the
Hls
global.The text was updated successfully, but these errors were encountered: