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 light ESM build #6307

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix light ESM build #6307

wants to merge 1 commit into from

Conversation

Chocobozzz
Copy link
Contributor

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 of undefined to prevent a crash.

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict

ESM build needs a special empty file that exports an object to prevent
property access of "undefined"
Copy link
Collaborator

@robwalch robwalch left a 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

@Chocobozzz
Copy link
Contributor Author

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 export in an if so I'm not sure how to conditionally export some modules

@robwalch
Copy link
Collaborator

robwalch commented Mar 27, 2024

@Chocobozzz,

It looks like only these types are exported with this issue:

var KeySystemFormats = empty.KeySystemFormats;
var KeySystems = empty.KeySystems;
var SubtitleStreamController = empty.SubtitleStreamController;
var TimelineController = empty.TimelineController;
...

I'm not sure why the others are not. Maybe they are being tree-shaken, and we just need to figure out why these are not?

@robwalch
Copy link
Collaborator

var KeySystemFormats = empty.KeySystemFormats;
var KeySystems = empty.KeySystems;
var SubtitleStreamController = empty.SubtitleStreamController;
var TimelineController = empty.TimelineController;

These exports are present because they are in exports-named.ts :

export { SubtitleStreamController } from './controller/subtitle-stream-controller';
export { TimelineController } from './controller/timeline-controller';
export { KeySystems, KeySystemFormats } from './utils/mediakeys-helper';

The issue is that we're using this same file as the input for the light esm build:

hls.js/build-config.js

Lines 343 to 348 in 0216391

lightEsm: buildRollupConfig({
input: './src/exports-named.ts',
type: BUILD_TYPE.light,
format: FORMAT.esm,
minified: false,
}),

@Chocobozzz
Copy link
Contributor Author

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.

@robwalch
Copy link
Collaborator

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?

@Chocobozzz
Copy link
Contributor Author

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 :)

@robwalch
Copy link
Collaborator

robwalch commented Apr 4, 2024

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 :)

We did #5486 to solve #5484.

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.

@robwalch
Copy link
Collaborator

robwalch commented Apr 4, 2024

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.

@robwalch robwalch self-requested a review April 4, 2024 20:23
@Chocobozzz
Copy link
Contributor Author

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.

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.

@robwalch robwalch added this to the 1.6.0 milestone Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants