Feat: exports for Base Plugin

[brian-byassee]

Short description

Exporting BasePlugin and render as named exports

Resolves #
Allows consumers to create custom plugins. Currently there are no cjs exports in the dist folder for base-plugin and render which causes linting to fail

Implementation details

How to test it

import WaveSurfer from 'wavesurfer.js'
import BasePlugin from 'wavesurfer.js/dist/base-plugin.js'
import createElement from 'wavesurfer.js/dist/dom.js'

console.log({ WaveSurfer, BasePlugin, createElement })

Screenshots

Checklist

• This PR is covered by e2e tests
• It introduces no breaking API changes

[katspaugh]

What implications does this have for the main WaveSurfer export? Will require('wavesurfer.js') work the same?

[jason-whitted]

@katspaugh It looks like it is modifying the wavesurfer.cjs file
Instead of module.exports=blah it will be exports.default=blah

I think this would impact NodeJS apps in having to change their import from:
const WaveSurfer = require('wavesurfer.js')
to:
const WaveSurfer = require('wavesurfer.js').default

But I could be wrong about that.

[katspaugh]

We should double-check that it’s not a breaking change. Especially for the min.js file which people tend to hot-load.

[brian-byassee]

Yeah, we checked and this is a breaking change for anyone using require to import wavesurfer

[katspaugh]

I would suggest not to add those exports to the main module then.
Why can’t one just import base plugin directly from dist?

https://unpkg.com/[email protected]/dist/base-plugin.js

[brian-byassee]

Eslint could not resolve the exports. The application would run but eslint would crash.

We updated the PR to export base-plugin and dom. Please take a look at the exports section in the package.json.

When there is a major version bump we should look at updating how exports are handled to make it easier to add new exports in the future. :)

[katspaugh]

This might backfire because the main bundle will have the same imports but a different copy thereof. So you would basically ship duplicate code, plus the class/type identity will differ. Wavesurfer expects plugins to be subclassesof that specific BasePlugin import, not another copy. Hope this makes sense, it barely does to me :)

[brian-byassee]

Yup I think this PR still has some issues beyond just that. Demoted it to a draft again and will look at it some more here in a bit. But I think I might just end up closing it for now.

More to come soon!

[katspaugh]

Looks like it's possible to ignore named exports for CommonJS modules via a rollup plugin.

Another alternative, just to acommodate the CommonJS module, would be to expose BasePlugin as a static property of the WaveSurfer class instead of exporting it as a named export.

[jason-whitted]

This might backfire because the main bundle will have the same imports but a different copy thereof. So you would basically ship duplicate code, plus the class/type identity will differ. Wavesurfer expects plugins to be subclassesof that specific BasePlugin import, not another copy. Hope this makes sense, it barely does to me :)

Just to clarify, this is already an existing problem. The current exports do not use the same BasePlugin class instance. Let me give an example.

import RegionsPlugin from 'wavesurfer.js/dist/plugins/regions.esm.js';
import TimelinePlugin from 'wavesurfer.js/dist/plugins/timeline.esm.js';

Both of these imports are distinct compilations of the source code with their own cjs/esm/min bundles. They contain the code for BasePlugin individually and are not referencing the same instance of the class.

This can be illustrated by running the following code:

console.log('same proto?', RegionsPlugin.__proto__ === TImelinePlugin.__proto__);

Or using instanceof:

const regions = new RegionsPlugin();
const timeline = new TimelinePlugin();
console.log('instanceof RegionsPlugin.__proto__?', regions instanceof RegionsPlugin.__proto__);
console.log('instanceof TimelinePlugin.__proto__?', regions instanceof TimelinePlugin.__proto__);

The way to fix this would be to modify the library to return a single bundle.

import WaveSurfer, { BasePlugin, TimelinePlugin, RegionsPlugin } from 'wavesurfer.js'

// Now TimelinePlugin.__proto__ === RegionsPlugin.__proto__ === BasePlugin

You can get this to work pretty easily by having a root index.js that contains the exports. Use rollup to build one single bundle and export it all. It lends itself to tree shaking this way, too.

But it would be a breaking change to people so this would require a major version bump.

[katspaugh]

True, and thanks for Index.ts pointer.

[katspaugh]

Sorry for the back and forth on this, but now that it's a static method of WaveSurfer, it would be good to not name it similar to the existing create method to avoid confusion when using autocomplete.

I suggest we do

import * as dom from './dom.js'

// ...

  public static readonly dom = dom

createElement could then be a named export.

And plugin authors could then use it like this:

const { createElement } = WaveSurfer.dom

WDYT?

[brian-byassee]

No worries at all dude! I updated the code.

* only imports named exports from a file. So I needed to update dom.js to have named export createElement.

I think we'd need to leave the default export since others maybe importing the default from dom.js in their projects.

[katspaugh]

Thank you, LGTM 👍