-
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
[WIP] feat: Split up code for modularization #2274
Conversation
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.
Nice 👍 Understood it's a WIP. I do like the idea of removing the light
build and considering the core as the light build.
While this PR does split out some of the advanced functionality code into a separate file, it doesn't go all the way to addressing #2175. There is the modularity angle to take into consideration as well. Example, what if we want subtitle support but don't want alt audio support?
Some ideas on this topic that I have come to mind:
Removing the usage of these build time constants (something you mentioned @Korilakkuma )
// TODO: Remove these envs
__USE_SUBTITLES__: false,
__USE_ALT_AUDIO__: false,
__USE_EME_DRM__: false
...and refactor how we check for existence and instantiate controllers to something where we can handle controllers generically.
Perhaps the static Hls
constructor can hold a "registry" of sorts where a "component" (simply a wrapper around a controller) can register itself. To start, by default the registry could hold today's core components. Later on that may change where we split out more functionality out of the core bundle.
Flow example:
- page loads
hls.core.js
Hls.registry
is now available on the global scope- auto-registers core components
- page loads `hls.subtitle-track.js' (a component)
- the component registers itself (e.g.
Hls.registry.add( ... )
)
- the component registers itself (e.g.
- page load complete
- page invokes
new Hls(config)
- internally, hls.js will retrieve all registered components
- e.g. storing instantiated components as a
Map<ComponentKey, ComponentController>
For public APIs such as get currentLevel
- that could look something like:
// Note: Code is verbose to show intentions
public get currentLevel () {
const controllers: Map<ComponentKey, ComponentController> = this._controllers
const streamController: StreamController = controllers.get(ComponentKey.Stream)
return streamController.currentLevel
}
Following that, we could refactor how we currently handle optional controllers (aka, controllers that now exist in the "extra" bundle)
// Note: Code is verbose to show intentions
get subtitleTracks () {
const controllers: Map<ComponentKey, ComponentController> = this._controllers
const subtitleTrackController = controllers.get(ComponentKey.SubtitleTrack);
if (subtitleTrackController) {
return subtitleTrackController.subtitleTracks
} else {
console.warn('Missing subtitle track component')
return [];
}
}
Thoughts @Korilakkuma ?
webpack.config.js
Outdated
splitChunks: { | ||
cacheGroups: { | ||
light: { | ||
test: /src\/controller\/(subtitle-stream|subtitle-track|timeline|audio-stream|audio-track|eme)-controller\.(js|ts)/, |
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.
While this does work to split out certain code to a separate file, I'm not a huge fan on the regular expression use here. Generally speaking this doesn't solve the modularity problem that #2175 is aiming to solve
Perhaps the naming of the chunk should change to better reflect the intention: light
-> hls.extra
. And on the same thought, perhaps renaming main
-> hls.core
or similar. Thoughts?
I haven't given this an in-depth look, but I believe that captions should be included in the core bundle. It's a heavily used feature. |
The 2nd PR (WIP) implements the following items.
So, I would like to have a review not details of changed code but directionality 🙏 |
@Korilakkuma - I did not forget about this 😄 I plan on reviewing soon :) I have been very busy lately which has caused me to be more or less absent on this project |
These changes aren't something I'm looking to bring into the current roadmap. See #2910 for details on new module exports. |
Following up on my last comment. I have been thinking about how we could achieve this with ESM modules and something like a composable player constructor (#2175 remains open). The difference between this attempt, is that I wouldn't want us to have to change the controllers or any of the core/network components. I would also want to avoid breaking up the library into multiple scripts. |
This PR will...
Split code into
core
andextra
(alt-audio support, subtitles support, EME support).But, this PR is a work in progress.
I would like to have a review not details of changed code but directionality.
Why is this Pull Request needed?
Please refer to #2175
Are there any points in the code the reviewer needs to double check?
Resolves issues:
#2175
Checklist
WIP