Modernizing peaks' package distribution

[jdelStrother]

Our app depends on waveform-data.js & peaks.js. Peaks inlines waveform-data into its distributed source file, which would normally result in waveform-data getting duplicated between our app & peaks. We've been hacking around that for a while by compiling peaks from its src directory:

  resolve: {
    alias: {
      "peaks.js": path.resolve(__dirname, "node_modules/peaks.js/src/main.js")
    }

In peaks 0.18 (or maybe 0.18.1), the src directory is no longer included in the npm distribution, so that doesn't seem to be an option any more.

Can we have an npm distribution that includes waveform-data as a peer dependency so we can de-dupe it?

[chrisn]

I'm not sure what the right solution is here, to be honest. For now, should I simply add the src files back in?

[jdelStrother]

I've been testing this some more today... last September, we were seeing waveform-data twice in our source files due to peaks.js inlining it, but that no longer appears to be the case. I'm not entirely sure if our webpack config changed or peaks did.

So we could just require the peaks.js & waveform-data packages normally, without our webpack hacks. However, I still think it would be worth including the src files in the package with something like this:

diff --git a/package.json b/package.json
index 7553e77..05056c7 100644
--- a/package.json
+++ b/package.json
@@ -3,8 +3,10 @@
   "version": "0.18.1",
   "description": "JavaScript UI component for displaying audio waveforms",
   "main": "./peaks.js",
+  "module": "./src/main.js",
   "types": "./peaks.js.d.ts",
   "files": [
+    "src",
     "peaks.js",
     "peaks.js.d.ts"
   ],

would would allow webpack & other bundlers to use the module entry to compile from source, which seems to do a better job of minifying the resulting package (from 298KB minified javascript to 281KB).

[chrisn]

The main thing I changed was to add the "files" field, as I noticed the npm package included the demo pages with audio files, making the package much larger than necessary. I also removed the "browser" entry in package.json and changed all the AMD dependencies, so I hope this change didn't break your build.

I'm planning to make another change to include only the Konva.js modules that we actually use, rather than the whole bundle. This should also help with file size, so I can also do this at the same time as this fix.

If I create a branch, would you mind testing before I make a new release?

[jdelStrother]

Sure, sounds good

[chrisn]

I have created a branch that adds the src directory. I haven't included the module entry, as the Webpack and Rollup documentation say this module should use ES2015 module syntax. Should we be switching from browserify to Webpack or Rollup?

I tried changing how we're importing Konva, but this broke the marker customisation demo page. More investigation needed.

[jdelStrother]

That branch seems fine (tested by cloning it, running npm pack, then building our app against the resulting peaks.js-0.18.1.tgz).

I started on a branch that converts all the AMD requires to ES6 imports, and bundles it with rollup - https://github.com/bbc/peaks.js/compare/master...jdelStrother:es6-rollup?w=1

Karma isn't working with it yet, but if you like I'll carry on prodding it.

[chrisn]

I think perhaps I should merge this change as is, so you have a working build again.

I'd like to try moving Konva.js to peerDependencies, so see if this solves the marker customisation problem.

The broader plan I have is to migrate the whole codebase to ES6 modules (per your branch), possibly also TypeScript. I experimented with Webpack, which seemed to work, I haven't tried Rollup. Possibly now is the time to do that

[jdelStrother]

I think perhaps I should merge this change as is, so you have a working build again.

👍

I experimented with Webpack, which seemed to work, I haven't tried Rollup

I'm a big fan of Webpack for building our own app, but it seems like much of the JS ecosystem recommends Rollup for building packages for other people to ingest. I don't have a strong opinion either way for Peaks.

[chrisn]

Peaks.js build now breaks when Konva.js is updated to 8.0.0

> peaks.js@0.26.0 prebuild
> npm run lint

> peaks.js@0.26.0 lint
> eslint src/*.js test/**/*.js test/*.js karma.conf.js

> peaks.js@0.26.0 build
> browserify -d -e ./src/main.js -t deamdify -s peaks | exorcist peaks.js.map | derequire - > peaks.js

/home/chrisn/Workspace/peaks.js/node_modules/konva/lib/index.js:1
import { Konva } from './_FullInternals.js';
^
ParseError: 'import' and 'export' may appear only with 'sourceType: module'
The code that you piped into exorcist contains no source map!
Therefore it was piped through as is and no external map file generated.
internal/streams/writable.js:285
      throw new ERR_INVALID_ARG_TYPE(
      ^

TypeError [ERR_INVALID_ARG_TYPE]: The "chunk" argument must be of type string or an instance of Buffer or Uint8Array.
Received an instance of Array
    at SyncWriteStream.Writable.write (internal/streams/writable.js:285:13)
    at /home/chrisn/peaks.js/node_modules/derequire/bin/cmd.js:33:18
    at ConcatStream.<anonymous> (/home/chrisn/peaks.js/node_modules/concat-stream/index.js:37:43)
    at ConcatStream.emit (events.js:388:22)
    at finishMaybe (/home/chrisn/peaks.js/node_modules/readable-stream/lib/_stream_writable.js:630:14)
    at endWritable (/home/chrisn/peaks.js/node_modules/readable-stream/lib/_stream_writable.js:638:3)
    at ConcatStream.Writable.end (/home/chrisn/peaks.js/node_modules/readable-stream/lib/_stream_writable.js:594:41)
    at Socket.onend (internal/streams/readable.js:684:10)
    at Object.onceWrapper (events.js:482:28)
    at Socket.emit (events.js:388:22) {
  code: 'ERR_INVALID_ARG_TYPE'
}

I'm currently looking into whether I can fix the browserify command line or if we need to move to rollup.

[chrisn]

I have just pushed a branch that replace Browserify with Rollup: https://github.com/bbc/peaks.js/tree/rollup

This shows the improvement in file size we get from using Rollup:

Bundle size	Minified size	Branch
622237 bytes	N/A	https://github.com/bbc/peaks.js/tree/master at https://github.com/bbc/peaks.js/commit/fca2787870bebe68f80eda002c52e64a1fb65a1e
586348 bytes	269337 bytes	https://github.com/bbc/peaks.js/tree/rollup
451564 bytes	208372 bytes	https://github.com/bbc/peaks.js/tree/konva-imports

The https://github.com/bbc/peaks.js/tree/konva-imports branch changes the Konva imports to only pull in the objects that Peaks.js actually uses. This reduces the bundle size further, but breaks the custom markers demo page (source). Would using peerDepencencies (with Rollup or Webpack to build the demo page JS bundle) solve this, or do I need to ensure the full Konva library is always included?

My naive attempt to fix the demo page by including Konva in a <script> tag didn't work!

[jdelStrother]

Yeah, Konva seems to get quite upset if you import two versions of it (it's one of the reasons I can't use peaks' precompiled javascript in my app and instead point webpack at node_modules/peaks.js/src/main.js).

Would using peerDepencencies (with Rollup or Webpack to build the demo page JS bundle) solve this

Yeah, I think it's probably the best option, shame about the extra re-tooling you'd need on the demo pages. The only other thing I can think is to expose peak's Konva shapes on the peaks instance itself - eg

--- a/demo/custom-markers.html
+++ b/demo/custom-markers.html
@@ -191,14 +191,14 @@
         CustomSegmentMarker.prototype.init = function(group) {
           this._group = group;

-          this._label = new Konva.Label({
+          this._label = new Peaks.KonvaLabel({
             x: 0.5,
             y: 0.5
           });

           var color = this._options.segment.color;

-          this._tag = new Konva.Tag({
+          this._tag = new Peaks.KonvaTag({
             fill:             color,
             stroke:           color,
             strokeWidth:      1,

but you're then limited to only using the Konva stuff that Peaks explicitly decides to provide. Not sure it's a good option...

[chrisn]

Thanks. I don't think Peaks should expose all the Konva objects, if we can avoid it. Next step here is to try out peerDependencies and fix the demo page. Not everybody uses the custom markers feature, so it would be good if they can benefit from the smaller bundle size.

[jdelStrother]

I don't think Peaks should expose all the Konva objects, if we can avoid it

Agreed, it doesn't look like a nice approach. I think it would be better to focus on updating the codebase to use es-modules so that people using bundlers can benefit from tree shaking and de-duping Konva/EventEmitter/etc, but I suspect that still leaves the precompiled peaks.min.js containing all of its dependencies. I think peerDependencies is a subsequent step on from that, but not 100% sure how it ought to work - are EventEmitter & WaveformData going to be peered as well?

[chrisn]

I have just pushed an update to the custom markers demo, which Is now built using Rollup.

https://github.com/bbc/peaks.js/tree/konva-imports/demo/custom-markers

This seems to work OK, but changing my React example as per https://github.com/dutzi/peaksjs-react-example/commit/98f5a805014054395e52206a79ad0508de9de0c5 doesn't work. I have the same issue described in https://github.com/bbc/peaks.js/issues/368.

If it makes any difference, I am testing using npm link from the peaksjs-react-example project to pull in my local peaks.js development copy. I've tried using peerDependencies, and adding these lines to package.json:

"main": "./src/main.js",
"exports": "./src/main.js",
"type": "module",

and no luck so far.

[jdelStrother]

I managed to get that working after a bunch of trial & error:

• make sure you're using npm 6 rather than 7 - 7 doesn't work with npm link'd dependencies (https://github.com/npm/cli/issues/2339)

• move konva from dependencies to peerDependencies in peaks.js, and add konva as a dependency to peaksjs-react-example

• for good luck, rm -rf node_modules in both peaks.js & peaksjs-react-example

• run npm link in peaks.js, and npm link peaks.js in peaksjs-react-example

• run npm install in peaksjs-react-example

Hopefully at this point you have one single instance of konva in peaksjs-react-example/node_modules (and none in peaksjs-react-example/node_modules/peaks.js/node_modules), and can drag custom markers around

[chrisn]

Thanks! Yes, deleting node_modules and reinstalling fixed it.

[chrisn]

This is all now implemented and merged to master. Is there a recommended way to reference the UMD bundles peaks.js and peaks.min.js from package.json?

[jdelStrother]

I'm surprised there's not an obvious best-practices-for-browser-package-authors guide somewhere, or at least not one I've managed to find. I think I would lean towards:

"main": "peaks.min.js",
"module": "src/main.js"

with no types field.

[chrisn]

I found this: https://github.com/rollup/rollup/wiki/pkg.module, which agrees with you. Let's try it!

[adi518]

I came here after paying a visit to https://bundlephobia.com/package/peaks.js@0.26.0. As you can see, it's currently sitting pretty high. This means it doesn't externalize its dependencies and costs double that size to consume, because when a consumer installs this library in their app, the bundler installs its dependencies and later on when you build your app, it will bundle the library dependencies into the app's bundle, even though it's already bundled into peaks, hence twice the size. I looked at the rollup branch, which is a step in the right direction (Browserify is really antique at this point and shouldn't be used). However, it misses the crux of the matter, as it provides an empty array to config.external (https://github.com/bbc/peaks.js/blob/rollup/rollup.config.js#L27). Since this library is vanilla, you can save a lot of work and effort, by building it with TSDX: https://tsdx.io/, which is based on Rollup. Looking at the source of this library, it can end up as little as ~5KB after fixing this issue.

[adi518]

I forked and applied TSDX. It's now 16.38KB instead of 70.4KB. TSDX also gives a basis for TypeScript (without much overhead), should you want to use it at some point.

Edit: Went for a smaller fix (that excludes a major infra change to TSDX), please review @chrisn: https://github.com/bbc/peaks.js/pull/410.

I'm surprised there's not an obvious best-practices-for-browser-package-authors guide somewhere, or at least not one I've managed to find. I think I would lean towards:

"main": "peaks.min.js",
"module": "src/main.js"

with no types field.

Without the types field, TS will complain about missing types for this library, so not a good idea. It's also unrelated to the purpose of main and module.

[chrisn]

@adi518 I have a demo app that uses Webpack to bundle Peaks.js as dependency. That project also depends on Konva.js, but there's no duplication. If I add waveform-data as a dependency, it is also not included as a duplicate. You must be doing something different, as I haven't seen the problem you describe.

[chrisn]

Without the types field, TS will complain about missing types for this library, so not a good idea. It's also unrelated to the purpose of main and module.

That was just a typo in the comment you quoted, the suggestion was to remove the type field.

[adi518]

@chrisn Konva is a peer dependency, so it's indeed not included in the bundle, however you have these other dependencies, which are not externalised and cause duplications, with the latter being the majority of the duplication, since eventemitter3 is really tiny.

"dependencies": {
  "eventemitter3": "~4.0.7",
  "waveform-data": "~4.1.0"
}

Pull the latest changes from #410 and run the following commands (after npm install):

1. npm run build-demo
2. npm run size

Then, run these:

1. npm run build
2. npm run size

You'll see the difference in bundle size immediately.

[chrisn]

I think this is complete, now we're using Rollup to create distribution bundles.