Migrate to Rollup with new ESM target and UMD Worker injection #5299

robwalch · 2023-03-15T23:08:19Z

This PR will...

Migrate from Webpack to Rollup, and improve build output and worker options.

Adds new config option workerPath (defaults to null)
- Adds output "hls.worker.js" as an (optional) alternative path to worker setup for developers not using the UMD library by pointing workerPath to this script
Adds ES Module output "hls.mjs"
Payload size is reduced (hls.min.js is roughly 369K vs 409K with webpack and new features in v1.4.0)
UMD builds are transpiled to ES5 with CI checks

Why is this Pull Request needed?

Webpack 5 boilerplate was not getting transpiled to ES5 (#5288). Rollout payload is smaller, and presents an opportunity to address several library output issues such as worker injection (#5107) and es module output (#2910).

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

Karma runner issues:
- Manual multi-entry for tests is not ideal and there could be issues with unit test watching.
- rollup pre-processor outputs temp files and if not given a file name and dir can overwrite the input file(s)

Resolves issues:

Resolves Provide an ES Modules build for modern browsers/tools #2910
Resolves Worker source handoff is fragile and likely to break with bundlers/minifiers #5107
- Similar to vue + hls.js bug , Uncaught TypeError: Cannot read properties of undefined (reading 'call') #5146
- Similar to Hls.js does not work with SvelteKit (vite) #5156
Includes fix for hls.js (and hls.min.js) are not being compiled to ECMAScript5 #5288

tjenkinson · 2023-03-16T17:29:16Z

This is awesome. Will have a proper look tomorrow

robwalch · 2023-03-16T18:43:56Z

Added Add ES5 syntax check for UMD builds #5301
package-lock needs an update

cloudflare-pages · 2023-03-17T15:22:58Z

Deploying with Cloudflare Pages

Latest commit:	`0097653`
Status:	✅ Deploy successful!
Preview URL:	https://aeee2168.hls-js-e5a.pages.dev
Branch Preview URL:	https://task-rollup-esm-and-worker.hls-js-e5a.pages.dev

View logs

tjenkinson

Hope you don't mind that I pushed a few commits? Look ok?

This looks great though I prefer rollup a lot more than webpack

Will look into getting the es-check bit passing now

tjenkinson · 2023-03-17T15:24:07Z

package.json

  "files": [
+    "demo/**/*",


Do we need to include the demo in the package? Not sure if it's useful?

Noticed we're also including the demo build in dist/ in the package. If we don't do this, we probably shouldn't do that either

package.json

tjenkinson · 2023-03-17T15:25:52Z

src/demux/inject-worker.ts

+import transmuxerWorker from './transmuxer-worker';
+
+if (hasUMDWorker() && typeof __IN_WORKER__ !== 'undefined' && __IN_WORKER__) {
+  transmuxerWorker(self);


I made some changes in here. Still look ok?

Previously this was assigned to a variable that was unused, and hasUMDWorker was checking if transmuxerWorker was defined, and I think it always would be?

I think we could take out hasUMDWorker() from the check above. It's really only needed in transmuxer-interface for injection.

tjenkinson · 2023-03-17T15:27:40Z

tests/index.js

@@ -1,3 +1,45 @@
 import 'promise-polyfill/src/polyfill';
-const testsContext = require.context('./unit', true);
-testsContext.keys().forEach(testsContext);
+import './unit/hls';


I think we should be able to dynamically generate this file with a plugin. Will probably look into that but this looks good for now

The alternative is to use something like https://github.com/rollup/plugins/tree/master/packages/multi-entry

itsjamie · 2023-03-17T15:49:17Z

The output from the esm build doesn't look like I would expect.

It still is wrapped in a commonjs output tree.

I would expect to see export keyword used. Right now, a user will still need to use something that converts the commonjs export into valid ES module format.

Switching to rollup, and the change from webpack definitely resolves a lot of issues, and I don't think this should block that.

But I'm not sure this resolves #2910 as is right now.

tjenkinson · 2023-03-17T15:51:21Z

The output from the esm build doesn't look like I would expect.

It still is wrapped in a commonjs output tree.

I this may be something I broke, checking

tjenkinson · 2023-03-17T15:59:33Z

@itsjamie yep I broke it 🤦

Look better now?

tjenkinson · 2023-03-17T16:06:07Z

rollup pre-processor outputs temp files and if not given a file name and dir can overwrite the input file(s)

This might still be a problem, and another one I made worse with my commits. Will check

itsjamie · 2023-03-17T16:32:47Z

The ESM build looks like I expect!

The implementation of injectWorker and hasUMDWorker and the code in the transmuxer means that an ESM user has no mechanism to actually use HLS.js with a Worker. I think the comment that #2910 isn't closed is still true. However, we're so much closer and it solves a plethora of problems with the other various bundlers that couldn't handle the webpack module code.

💯

robwalch · 2023-03-17T17:21:22Z

The implementation of injectWorker and hasUMDWorker and the code in the transmuxer means that an ESM user has no mechanism to actually use HLS.js with a Worker. I think the comment that #2910 isn't closed is still true. However, we're so much closer and it solves a plethora of problems with the other various bundlers that couldn't handle the webpack module code.

It would help if we had a page that demoed the ESM output. Do we expect developers to use the esm module at runtime or to import and bundle themselves? both?

I'd like to get it to where hasUMDWorker() is only used in UMD builds. For an ESM build if we can detect the context (window vs worker) the library running in a worker should always kick off transmuxer-worker. In the main event loop we would want either has an iife that could be used with injectWorker or an option to specify the URL of the library for loading in a worker.

gkatsev · 2023-03-17T18:09:11Z

It would help if we had a page that demoed the ESM output. Do we expect developers to use the esm module at runtime or to import and bundle themselves? both?

I think it's reasonable for this PR to only target bundle users rather than native ESM.

gkatsev · 2023-03-17T18:44:03Z

Did a quick filesize check and seems to have improved slightly:

In master

Filename	Filesize	Gzipped
hls.js	1.1M	246K
hls.min.js	396K	112K

In this PR

Filename	Filesize	Gzipped
hls.js	970K	218K
hls.min.js	358K	106K

gkatsev

Testing it out locally, and when loading in the new esm build, it seems to work fine in a pure-client-side app, but in a nextjs app it fails to build with an error about self not being available as part of self.WebKitDataCue || self.VTTCue || self.TextTrackCue. I'm still investigating whether it's an hls.js issue or an us issue, but thought I'd post here in the meantime as well.

package.json

robwalch · 2023-03-17T20:37:50Z

rebased against latest

tjenkinson · 2023-03-17T20:44:21Z

It's blatted the commits I added. I'll bring them back on another branch

tjenkinson · 2023-03-17T21:04:33Z

Brought them back here #5309

tjenkinson · 2023-03-17T21:49:26Z

in a nextjs app it fails to build with an error about self not being available as part of self.WebKitDataCue || self.VTTCue || self.TextTrackCue.

8b84d44 should fix that

robwalch · 2023-03-17T22:52:42Z

We replaced all reference to window with self at some point to prevent issues in the worker context. Is there another alternative that would work in @gkatsev's test case with nextjs? (We've also had issues where self is undefined or overwritten. Not sure if the rollup out formats can do something with global/globalThis/this or something like import global from 'global' which I've seen in rollup repo)

gkatsev · 2023-03-17T23:00:52Z

Can confirm that 8b84d44 fixes it.

robwalch · 2023-03-17T23:06:36Z

Just so I understand, this is when HLS.js is run in nodejs, presumably as part of unit tests?

Could we add a build step that tests this?

robwalch · 2023-03-25T17:56:15Z

Looking for approval to merge and feedback on 39682b6:

Add "workerPath": "../dist/hls.worker.js" to PR preview.

Did some in browser testing of hls.mjs and looks OK to me. If ESM usage, demo, and testing are important to you please contribute the changes you would like to see or need.

tjenkinson · 2023-03-25T19:57:59Z

LGTM!

Just made a couple more tweaks in #5340

tjenkinson · 2023-03-25T20:05:39Z

Actually the cloudflare pages demo isn't working?

https://1f937422.hls-js-4zn.pages.dev/demo/

tjenkinson · 2023-03-25T20:31:44Z

The worker was not being included in the bundle. I fixed it in #5340

robwalch · 2023-03-26T17:58:26Z

build-config.js

+      ...basePlugins,
+      replace(buildConstants(type)),
+      ...(!shouldBundleWorker(format)
+        ? [alias({ entries: { './transmuxer-worker': '../empty.js' } })]


@tjenkinson,

Is it worth removing this from the esm build? Is there a way we could leave this to tree-shaking?

Would anyone want to set IN_WORKER and HLS_WORKER_BUNDLE themselves with the esm output?

I think we’d need to export the worker function and then import that function in inject-worker, and then make sure it’s referenced inside an if statement in a way that would cause rollup to not tree shake it away, which is a bit messy

Also not sure what the benefit of it being in there would be, as it would never actually run?

Also not sure what the benefit of it being in there would be, as it would never actually run?

I was thinking someone could use the module to produce a single output that could be used in both contexts as this project does. That's more complicated than it needs to be however.

Add an es module target (#2910) UMD build worker injection (#5107) Add ES5 syntax check for UMD builds (#5301, #5288)

remove unused `workerInstance` variable refactor rollup config deduped some of the config and configured rollup to bail on any warning Also renamed the command line arg to `configType` as rollup reserved `config*` for custom configs, and won't throw a warning that an unknown property was passed with that downgrade rollup config to cjs and split into 2 files so we can share the config with karma use same rollup config definition for tests and actual build bring the dev script back do not use `const` in `umdBanner` leave test streams file as commonjs but allow it to be imported

which happens in nodejs. The check that makes sure the dist file can be required in node and not throw was failing because of this. Probably only an issue now due to how rollup hoists things

Disable sourcemap in tests Optimize babel settings Rename __HLS_WORKER_BUNDLE__ worker iffe wrapper and remove __HLS_UMD_WORKER__ Don't null config on destroy as it violates public API type defintion

Resolves #5107

This reverts commit c23df80.

…tup and error logs

robwalch requested a review from itsjamie March 15, 2023 23:08

robwalch force-pushed the task/rollup-esm-and-worker branch from ef48950 to e8698e1 Compare March 15, 2023 23:39

robwalch mentioned this pull request Mar 15, 2023

hls.js (and hls.min.js) are not being compiled to ECMAScript5 #5288

Closed

5 tasks

robwalch requested review from tjenkinson and cjpillsbury March 15, 2023 23:53

This was referenced Mar 16, 2023

Worker source handoff is fragile and likely to break with bundlers/minifiers #5107

Closed

Provide an ES Modules build for modern browsers/tools #2910

Closed

robwalch mentioned this pull request Mar 16, 2023

Add ES5 syntax check for UMD builds #5301

Merged

3 tasks

tjenkinson reviewed Mar 17, 2023

View reviewed changes

gkatsev reviewed Mar 17, 2023

View reviewed changes

package.json Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

robwalch force-pushed the task/rollup-esm-and-worker branch 2 times, most recently from 915594a to 9eb09ee Compare March 17, 2023 20:35

robwalch commented Mar 26, 2023

View reviewed changes

tjenkinson previously approved these changes Mar 27, 2023

View reviewed changes

robwalch dismissed tjenkinson’s stale review via 7b373ab March 27, 2023 16:42

robwalch and others added 18 commits March 27, 2023 09:51

Convert build packager to rollup

b242767

Add an es module target (#2910) UMD build worker injection (#5107) Add ES5 syntax check for UMD builds (#5301, #5288)

update __NETLIFY__ to __CLOUDFLARE_PAGES__

4b9226e

remove the self = this

9be7ad4

handle self not existing

9db9316

which happens in nodejs. The check that makes sure the dist file can be required in node and not throw was failing because of this. Probably only an issue now due to how rollup hoists things

FIx unit test coverage report

f7f30b7

Disable coverage in CI

b39f802

Disable sourcemap in tests Optimize babel settings Rename __HLS_WORKER_BUNDLE__ worker iffe wrapper and remove __HLS_UMD_WORKER__ Don't null config on destroy as it violates public API type defintion

Cleanup fragment-loader in unit tests

f75b4de

Add config.workerPath option and "hls.worker.js" build output

a1a336c

Resolves #5107

do not use banner/footer for worker build

c359c4f

remove outputFile option

0259564

Revert "remove outputFile option"

485423d

This reverts commit c23df80.

add temp dir for karma watch output to gitignore

16ca960

ensure the worker ends up in the bundle

adfb55e

Remove demo from package

abb05a2

Remove webpack config

76c0d7d

Include transmuxer-interface id ("main" and "audio") in Web Worker se…

fec0947

…tup and error logs

Change karma unit test concurrency

536dad2

robwalch force-pushed the task/rollup-esm-and-worker branch from 7b373ab to 536dad2 Compare March 27, 2023 16:56

robwalch merged commit 8e6efb1 into master Mar 27, 2023
15 of 16 checks passed

robwalch deleted the task/rollup-esm-and-worker branch March 27, 2023 18:03

robwalch mentioned this pull request Apr 11, 2023

vue + hls.js bug , Uncaught TypeError: Cannot read properties of undefined (reading 'call') #5146

Closed

5 tasks

robwalch mentioned this pull request Jul 3, 2023

Playback starts from 3 seconds only in compiled form (vite) #5617

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Rollup with new ESM target and UMD Worker injection #5299

Migrate to Rollup with new ESM target and UMD Worker injection #5299

robwalch commented Mar 15, 2023 •

edited

tjenkinson commented Mar 16, 2023

robwalch commented Mar 16, 2023 •

edited by tjenkinson

cloudflare-pages bot commented Mar 17, 2023 •

edited

tjenkinson left a comment

tjenkinson Mar 17, 2023

tjenkinson Mar 17, 2023

tjenkinson Mar 17, 2023

robwalch Mar 17, 2023

tjenkinson Mar 17, 2023

gkatsev Mar 17, 2023

itsjamie commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

itsjamie commented Mar 17, 2023

robwalch commented Mar 17, 2023

gkatsev commented Mar 17, 2023

gkatsev commented Mar 17, 2023

gkatsev left a comment

robwalch commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

robwalch commented Mar 17, 2023

gkatsev commented Mar 17, 2023

robwalch commented Mar 17, 2023 •

edited

robwalch commented Mar 25, 2023

tjenkinson commented Mar 25, 2023

tjenkinson commented Mar 25, 2023

tjenkinson commented Mar 25, 2023

robwalch Mar 26, 2023

tjenkinson Mar 26, 2023

robwalch Mar 27, 2023

Migrate to Rollup with new ESM target and UMD Worker injection #5299

Migrate to Rollup with new ESM target and UMD Worker injection #5299

Conversation

robwalch commented Mar 15, 2023 • edited

This PR will...

Why is this Pull Request needed?

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

Resolves issues:

tjenkinson commented Mar 16, 2023

robwalch commented Mar 16, 2023 • edited by tjenkinson

cloudflare-pages bot commented Mar 17, 2023 • edited

Deploying with Cloudflare Pages

tjenkinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsjamie commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

itsjamie commented Mar 17, 2023

robwalch commented Mar 17, 2023

gkatsev commented Mar 17, 2023

gkatsev commented Mar 17, 2023

gkatsev left a comment

Choose a reason for hiding this comment

robwalch commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

tjenkinson commented Mar 17, 2023

robwalch commented Mar 17, 2023

gkatsev commented Mar 17, 2023

robwalch commented Mar 17, 2023 • edited

robwalch commented Mar 25, 2023

tjenkinson commented Mar 25, 2023

tjenkinson commented Mar 25, 2023

tjenkinson commented Mar 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robwalch commented Mar 15, 2023 •

edited

robwalch commented Mar 16, 2023 •

edited by tjenkinson

cloudflare-pages bot commented Mar 17, 2023 •

edited

robwalch commented Mar 17, 2023 •

edited