indigo-player icon indicating copy to clipboard operation
indigo-player copied to clipboard

Using with webpack/ES6 imports

Open inversion opened this issue 4 years ago • 7 comments

Is your feature request related to a problem? Please describe. It seems that to use this project with a webpack build, the built and minified script lib/indigo-player.js needs to be loaded with script-loader (or just added as a

Describe the solution you'd like It would be nice to be able to import indigo-player as an ES6 module to gain typings, avoiding using the global and integrate into our normal chunk lazy-loading workflow e.g

import IndigoPlayer from 'indigo-player';

IndigoPlayer.init()

Currently this generates a lot of TypeScript errors for me (TS 3.5.1):

10:01:21 0|tsc      | node_modules/indigo-player/src/types.ts(334,3): error TS7010: 'create', which lacks return-type annotation, implicitly has an 'any' return type.
10:01:21 0|tsc      | node_modules/indigo-player/src/types.ts(360,3): error TS7010: 'on', which lacks return-type annotation, implicitly has an 'any' return type.
10:01:21 0|tsc      | node_modules/indigo-player/src/types.ts(361,3): error TS7010: 'once', which lacks return-type annotation, implicitly has an 'any' return type
...

Happy to help work on this or provide more info if it's unclear.

inversion avatar Nov 29 '19 10:11 inversion

I think the reason for this issue is because of how index.ts and index.d.ts mismatch type declarations.

lxsmnsyc avatar Dec 17 '19 15:12 lxsmnsyc

The main issue is that indigo is not a single module, but consists of multiple chunks (webpack includes these with the impact("...") method), and I'm not really sure how that would work yet.

One way I can think of is to let webpack build a single module for NPM, ignoring multiple chunks.

Another is to drop chunks overall and serve a single JS file.

If I'm not mistaken, you're getting these errors because you have strict mode in your TS config. I think enabling strict mode in indigo would be the first step forward. I can surely work on this, but happy to receive some help here. :)

matvp91 avatar Dec 19 '19 20:12 matvp91

What I did to solve this was through code-splitting, in which I load up your lib files in a module that is to be imported through dynamic imports (see NextJS's dynamic import), and I have to write type declaration for it to work. Problem is, there are instances that the SSR library will throw an error (I think this is because of how the module, implemented in UMD(?), requires the window object to attach it's namespace.

It was very painful to write with:

import IndigoPlayer from 'indigo-player';

import 'indigo-player/lib/UiExtension.a7474485823df0c37e29';
import 'indigo-player/lib/HlsMedia.224196f573ab35d7a1bb';
import 'indigo-player/lib/DashMedia.b648736869df1ff4cdc4';
import 'indigo-player/lib/ThumbnailsExtension.0a3a0c8ae41565501670';

the css file cannot be imported so I have to use the CDN service as a workaround.

lxsmnsyc avatar Dec 20 '19 06:12 lxsmnsyc

after a painful month of analyzing this repo, I finally succeeded on doing a rewrite.

https://github.com/LXSMNSYC/indigo-player-2

There were problems in your current codebase that are related on how the package was written i.e. dependency cycles, verbose webpack code-splitting, React-related issues, not so type-safe Typescript etc., which I bothered on solving.

Features:

  • no Sass stylesheets (so users can implement it easily).
  • can now be used as a module (yay!)
  • handles code-splitting in a much better way.
  • much better dev experience (hopefully)

Cons:

  • no theming (sadly).

lxsmnsyc avatar Jan 13 '20 11:01 lxsmnsyc

@LXSMNSYC damn, that's a whole lot of work you've done! Could you elaborate on your comment a bit more? Know that indigo was never meant to be used as a module because of the fact that it relies heavily on chunking.

I've made a mistake not to go with TS strict mode from the beginning, I totally agree that this would be a pain for people to get into.

Know that indigo was built with the idea of a single entry. Drop in indigo-player.js and it just works, it takes care of the related chunks, fetching the proper CSS theme and that's it. Changing this setup would break the public api, and I'd rather not do that. (breaking contracts would bump a major version).

As for your remarks:

  • React-related issues, could you point me to what you mean? As far as I can tell, the React UI is nothing complex and barely does state management (as it gets a whole lot of state from the facade).
  • What exactly is changed in terms of code splitting? Apart from splitting module definitions and the actual loading of a module (in async), I don't see much difference as what is described here: https://github.com/matvp91/indigo-player/blob/master/src/ModuleLoader.ts

How exactly would bundle splitting work with a module? Wouldn't this mean that the host app should have chunks built in in it's build process (aswell as providing a public path, or the player will not know where to get it's chunks. This is the case with the version hosted on jsdelivr CDN, which is built with the jsdelivr public path). Makes me think that it might be better to remove bundle splitting on the player side, and provide indigo as an ES5 module. The build process would then include feature flags (eg: dash, hls - which will include either shaka or hls.js in the bundle).

If you're interested, I'm more than open to accept PR's.

matvp91 avatar Jan 13 '20 11:01 matvp91

First off, I would like to address that the reasoning behind my own decision to rewrite your wonderful library was because due to the compliance of it being used as a module, as well as not to break the building pipeline we have. This was a race against time.

To have you informed, my team is using the Next.js library for serving our serverless web app. To deploy the app, we build the Next pages.

Seems easy enough, but that's where things went grim for our production:

  • Type defintions for the indigo-player threw errors when being parsed by tsc (when I used it).
  • Using the built indigo-player js file seems too hacky just to incorporate the package.
  • Importing the built css files was not a choice (we don't like modifying Next.js SSR template)

and a lot more issues that I can barely even count. (If there's a need for more information, I think I'll be able to list them down)

With indigo-player blocking the build pipeline, we have a choice to pull out indigo and choose something else.

But that thing is, we can't. We have not enough time to switch packages, and If I remember correctly, there are too few packages for video players that incorporate our needs. We found everything we need in indigo-player, and we just only need to fix it and make it work.

lxsmnsyc avatar Jan 13 '20 12:01 lxsmnsyc

With my revisions, indigo-player wouldn't be usable on CDN.

The thing is tho, modern web developers are now more inclined to use CommonJS/ES modules rather than to use IIFE modules.

Don't be mistaken on this replacing your package (and submitting a PR). IMO I chose to do this as part of my team's goal and preference.

lxsmnsyc avatar Jan 13 '20 12:01 lxsmnsyc