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

Also provide a light ES module (without the worker) #5484

Closed
Chocobozzz opened this issue May 12, 2023 · 11 comments · Fixed by #5486
Closed

Also provide a light ES module (without the worker) #5484

Chocobozzz opened this issue May 12, 2023 · 11 comments · Fixed by #5486

Comments

@Chocobozzz
Copy link
Contributor

Is your feature request related to a problem? Please describe.

With 1.4, minification using esbuild or terser broke the worker module (from hls.js or hls.light.js) when hls.js is bundled inside our client application (webpack bundled).

I was able to use the ES module with the worker thread using the following options:

enableWorker: true,
workerPath: URL.createObjectURL(new Blob([ require('!raw-loader!hls.js/dist/hls.worker.js').default ], { type: 'text/javascript' })),

Unfortunately current NPM dist files don't include a hls.light.mjs so our bundle size increased even if we don't need alternate-audio, subtitles, CMCD, EME (DRM), or Variable Substitution support features.

Describe the solution you'd like

Provide a hls.light.mjs dist file

Additional context

Thanks for your work, very appreciated 👏

@Chocobozzz Chocobozzz added Feature proposal Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. labels May 12, 2023
@robwalch robwalch removed the Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. label May 12, 2023
@robwalch
Copy link
Collaborator

Hi @Chocobozzz,

Would you consider contributing the change in the form of a PR?

The configuration you as asking for is missing from build-config.js:

hls.js/build-config.js

Lines 273 to 298 in 093a41c

const configs = Object.entries({
full: buildRollupConfig({
type: BUILD_TYPE.full,
format: FORMAT.umd,
minified: false,
}),
fullMin: buildRollupConfig({
type: BUILD_TYPE.full,
format: FORMAT.umd,
minified: true,
}),
fullEsm: buildRollupConfig({
type: BUILD_TYPE.full,
format: FORMAT.esm,
minified: false,
}),
light: buildRollupConfig({
type: BUILD_TYPE.light,
format: FORMAT.umd,
minified: false,
}),
lightMin: buildRollupConfig({
type: BUILD_TYPE.light,
format: FORMAT.umd,
minified: true,
}),

@Chocobozzz
Copy link
Contributor Author

Sure! #5486

@tjenkinson
Copy link
Member

Hey @Chocobozzz do you have the error message you get?

If it’s due to the wrapping function name getting mangled we might be able to fix that

@tjenkinson
Copy link
Member

If you have a link to the broken build so I can reproduce it that would be awesome

@Chocobozzz
Copy link
Contributor Author

Chocobozzz commented May 15, 2023

The broken build is on https://peertube2.cpy.re/videos/embed/tKQmHcqdYZRdCszLUiWM3V
The error: Uncaught ReferenceError: t is not defined (blob:https://peertube2.cpy.re/5c2a8fd9-df97-468c-8b7c-2342b01abc51:1)

The webpack configuration that provides such build is https://github.com/Chocobozzz/PeerTube/blob/develop/client/webpack/webpack.video-embed.js

You can re-create the build by:

  • cloning git@github.com:Chocobozzz/PeerTube.git
  • yarn install --pure-lockfile
  • building the embed: npm run build:embed

@tjenkinson
Copy link
Member

Thanks but I get a 401 on that url atm

@Chocobozzz
Copy link
Contributor Author

Arf sorry. Here is a public link: https://peertube2.cpy.re/videos/embed/aXCh29qwRKFFmuNVFNL7vT

@pasieronen
Copy link

Apparently the name that is being mangled by webpack/terser is module. The string inside injectWorker() where it's defined is not mangled, but the place where it's eventually used gets mapped to t (or some other single-letter name) - and thus I get Uncaught ReferenceError: t is not defined.

I was able to work around this by passing {mangle: {reserved: ['module']}} to terser.

@pasieronen
Copy link

Here's a very minimal broken build: https://github.com/pasieronen/hlsjs-terser-bug

@tjenkinson
Copy link
Member

Thanks @pasieronen that was super helpful.

So webpack without minification rewrites:

(function (global, factory) {
  console.log('factory', factory);
  console.log('typeof exports', typeof exports);
  console.log('typeof module', typeof module, module);
  console.log('typeof define', typeof define, define);
  console.log('global', global);

  typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() :
  typeof define === 'function' && define.amd ? define(factory) :
  (global = typeof globalThis !== 'undefined' ? globalThis : global || self, global.Hls = factory());
})(this, (function () { 'use strict';

to

(function (global, factory) {
  console.log('factory', factory);
  console.log('typeof exports', typeof exports);
  console.log('typeof module', "object", module);
  console.log('typeof define', "function", __webpack_require__.amdD);
  console.log('global', global);

   true ? module.exports = factory() :
  0;
})(this, (function () { 'use strict';

which when loaded in a worker would actually fail on console.log('typeof module', "object", module) because in reality module is undefined, along with __webpack_require__.

Without the console logs though the unminified one works because outside the worker there is a var module={}.

When webpack minifies though the module.exports becomes t.exports, but the var module stays as var module, so there's the mismatch.

So yep marking module as reserved in terser works around it.

I think a way for us to avoid this would be a custom amd header that also knows if it's running in the worker, which then could bail early and just run the factory function.

Something like

(function __HLSJS_BUNDLE__(inWorker, global, factory) {
  if (inWorker) { factory(); return; }
  
  //...
})(false, this, (function () { 'use strict';

where when the code is built for the worker __HLSJS_BUNDLE__ would be stringified and true passed in at that point. __HLS_WORKER_BUNDLE__ would be removed.

I've approved #5486 though because I think it makes sense to also offer that build :)

@tjenkinson
Copy link
Member

Actually I think we could move the __HLS_WORKER_BUNDLE__ inside the default wrapping function instead and then the worker wouldn't have to deal with any umd stuff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants