-
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 light ESM build #6307
Fix light ESM build #6307
Conversation
ESM build needs a special empty file that exports an object to prevent property access of "undefined"
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.
Rather than change the “empty” export, change the type exports in hls.ts to not export these types in a light/full build const conditional block
Do you have any hints on how to make this work? You can't wrap an |
It looks like only these types are exported with this issue:
|
These exports are present because they are in exports-named.ts : Lines 41 to 43 in 0216391
The issue is that we're using this same file as the input for the light esm build: Lines 343 to 348 in 0216391
|
We can use a different input for the light build that do not export these types/objects, but the drawback is that we don't use the variables/config injected by rollup. This means that if we want to add/remove other components from the light build, we would have 2 different places to update. |
So you prefer having the missing types stubbed like this? |
I have no preferences as I'm not involved enough in hls.js and my knowledge of rollup is limited, so I can implement whatever solution you think is best :) |
From this issue it seems that was never successful. I think we need an ES module entry point that composes a default config with the controllers you need, so that whatever you don't import in the entry is not included. This would require changing config.ts and hls.ts to be composable based on an entry point module like export-named that also defined a custom constructor extending Hls and a DefaultConfig with only the imported controllers. If that works then at least the aliases for constructor -> empty could be removed. Additional organization of utils and types to remove them from those dependency graphs would also be needed. |
If this issue is blocking you or resolving it would reduce your bundle size, we could accept this change and release it in a patch along with #6316. Let me know what the urgency if for you. I think this is certainly the simplest approach and acceptable for a patch. Fixes requiring new entry points like what I suggested above should be explored with new enhancement requests. |
This is currently blocking hls.js upgrade in PeerTube. But it's not a big deal as we''ll be moving to the full hls.js build in a few months as we plan to support m3u8 playlists that contain separate audio and video streams. My personal opinion is that we can merge this PR as it fixes the bug (ES version of hls.js light build is not usable at the moment) while still planning the better solution you mentioned, that requires more work. But having a composable hls.js library where the project that import it can choose which components to use and thus reduce the final bundle size would be awesome. |
ESM build needs a special empty file that exports an object to prevent property access of "undefined" (cherry picked from commit 474a10b)
* patch/v1.5.x: Fix light ESM build (#6307)
ESM build needs a special empty file that exports an object to prevent property access of "undefined"
This PR will...
Fix
hls.light.mjs
script crash (TypeError: Cannot read properties of undefined (reading 'KeySystemFormats')
Why is this Pull Request needed?
ESM bundle imports
empty
as an object. So we must export an empty object instead ofundefined
to prevent a crash.Are there any points in the code the reviewer needs to double check?
Resolves issues:
Checklist