RFC: npm package improvements

[WolfgangDrescher]

I have recently resumed the development of the Web Worker integration I started in #1315 (also see #1253). I have come to a point where it would be best if the package code (which is currently just a single file) was split into multiple files so that they could be imported into the worker file without any code repetitions. For this reason, I started to rethink the logic of npm packages for Verovio and came across some questions:

JavaScript build matrix

Why does buildToolkit build multiple versions of Verovio? What is the difference between the JS build and the npm build, except that the npm build exports a node module? What's the difference between the wasm and the non-wasm build? What is the difference between the light and the non-light version. Why do we need so many different versions and what are the use cases? The toolkit matrix in the GitHub workflow is quite big: https://github.com/rism-digital/verovio/blob/develop/.github/workflows/ci_build.yml#L257-L272. It looks like a wasm build with Humdrum support is missing in the ci_build.yml workflow. E.g. like this:

    strategy:
      matrix:
        toolkit:
          - target: nohumdrum
            message: "Building toolkit without humdrum"
            options: "-c -H -M"
            filepath: "verovio-toolkit.js*"
          - target: light
            message: "Building toolkit without humdrum as light version"
            options: "-c -H -l -M"
            filepath: "verovio-toolkit-light.js*"
          - target: wasm-nohumdrum
            message: "Building toolkit without humdrum as wasm"
            options: "-c -H -w -M"
            filepath: "verovio*wasm*"
          - target: wasm
            message: "Building toolkit as wasm"
            options: "-c -w -M"
            filepath: "verovio-toolkit-wasm-hum.js*"
          - target: default
            message: "Building default toolkit with humdrum"
            options: "-c -M"
            filepath: "*-hum.js*"

Bundler

If we decide to split the source code of the current index.js into multiple files, it would probably be a good idea to bundle the code again with a bundler like Webpack or rollup.js. What could speak against this? A bundler could also make sure that the file can be used in multiple environments: node with CommonJS, node with ESM or even directly in the browser. There is a nice example in the Vite documentation that shows how the files could be bundled. See this example for the package.json config:

{
  "name": "my-lib",
  "files": ["dist"],
  "main": "./dist/my-lib.umd.js",
  "module": "./dist/my-lib.es.js",
  "exports": {
    ".": {
      "import": "./dist/my-lib.es.js",
      "require": "./dist/my-lib.umd.js"
    }
  }
}

This could drastically increase the size of the npm package, since the Emscripten build would have to be included once as a source file and a second time inside the final bundled file. However, this could be solved with the files entry in package.json or a .npmignore file: https://docs.npmjs.com/cli/v7/configuring-npm/package-json#files.

In my second example in the next ESM section the Emscripten build would not be bundled, so the size of the bundle would not be a problem.

ESM

It would probably be a good idea to support ESM for the npm packages, as suggested in #2473. That would mean to bump engines.node in package.json to >= 12.0.0. But since Node.js v18 was released just some days ago, the current LTS version is v16, and v12 is only supported until 2022-04-30 I don't think this would be a problem. See https://nodejs.org/en/about/releases/.

ESM would even allow native integration directly in modern browsers, as pointed out in #2473 (https://caniuse.com/es6-module), and it is also possible to use the import and export syntax within a Node.js environment. This can be achieved by using the .mjs file extensions for single files, or by using "type": "module" in the package.json file to enable ESM for the entire package.

I found a configuration for the Emscripten build setp that should work: -s MODULARIZE=1 -s EXPORT_ES6=1. However, this seems to break the current setup for using the Verovio Toolkit in JavaScript, since with this flags the module needs to be created first and then returns a promise that needs to be awaited. But with the promise this seems to be a very straight forward way to set the module up! See https://stackoverflow.com/a/53384917/9009012. It could also solve some problems I had in hot reload mode with the onRuntimeInitialized event.

Something like this could be the new setup script:

import { VerovioWasmFactory, VerovioToolkit } from 'verovio';

VerovioWasmFactory().then(function(VerovioModule) {
    const verovioToolkit = new VerovioToolkit(VerovioModule);
    console.log(verovioToolkit.getVersion());
    verovioToolkit.loadData(score);
    const data = verovioToolkit.renderToSVG(1, {})
});

Another option could be like so:

import VerovioWasmFactory from 'verovio/wasm/verovio-toolkit-wasm-hum.js';
import { VerovioToolkit } from 'verovio';

VerovioWasmFactory().then(function(VerovioModule) {
    const verovioToolkit = new VerovioToolkit(VerovioModule);
    console.log(verovioToolkit.getVersion());
    verovioToolkit.loadData(score);
    const data = verovioToolkit.renderToSVG(1, {})
});

Or directly in a modern browser:

<script type="module">
    import VerovioWasmFactory from './verovio/wasm/verovio-toolkit-wasm-hum.js';
    import { VerovioToolkit } from './verovio/index.mjs';

    VerovioWasmFactory().then((VerovioModule) => {
        const verovioToolkit = new VerovioToolkit(VerovioModule);
        console.log(verovioToolkit.getVersion());
        verovioToolkit.loadData(score);
        const data = verovioToolkit.renderToSVG(1, {});
    });
</script>

I think the second option would be the best solution, although the import is a bit verbose with two statements. I have decoupled the dependency from a global Module variable and the toolkit now has to get the Module instance passed in the constructor. This would allow the toolkit to be bundled with a tool like rollup.js or Webpack, but the Emscripten build itself would not be included in that build since you have to import it separately. This would even allow multiple versions of the JS build (e.g. the current build matrix from the ci_build.yml pipeline) to be delivered within the same npm package, so the new verovio-humdrum npm package would no longer be needed. All built variants can be added into the wasm folder. This would only increase the file size of the verovio npm package, but that doesn't seem to become problematic for 20MB or so: npm/npm#12750 (comment). The bundle size of the developer application wouldn't explode either, since only the Module version they import will be bundled.

In any case, this would be a breaking change and therefore it would be a good idea to implement it with the next major release v4.0.0.

Web Worker

I've been researching Web Worker integration for npm packages and I think the best solution would be for Verovio to ship a file that developers can use to let their bundlers build the worker file for them.

The best example I found is from the Monaco Editor repository (Visual Studio Code). See https://github.com/microsoft/monaco-editor/blob/main/docs/integrate-esm.md and vitejs/vite#1791 (comment).

The Vite example (it is using rollup.js behind the scenes) for integrating the ESM version of the Monaco editor as a web worker is easy to understand. Vite can setup a file as a web worker by simply adding ?worker after the import statement. We should leave it to the verovio package users to create the worker, but provide a file from which they can generate it. E.g.:

import VerovioWorker from 'verovio/verovio.worker?worker';

window.VerovioEnvironment = {
    createWorker() {
         return new VerovioWorker();
    },
};

Insinde the verovio package we could check if the VerovioEnvironment.createWorker variable is present, if so create the toolkit as a worker and if not with native JavaScript:

createToolkitProxy() {
    return self.VerovioEnvironment?.createWorker ? this.createWorkerProxy() : this.createNativeProxy();
}

That was initially why I wanted to refactor the vervio npm package so that the 10MB Emscripts wasm build doesn't have to be included twice in the package, but it can be imported.

Without Vite or with another bundler, as far as I know, a worker can be created as follows: new Worker(new URL('./file/to/verovio.worker.js', import.meta.url)));. But I haven't tested this myself yet.

Since the worker must use async method calls on the toolkit (see https://github.com/WolfgangDrescher/verovio-worker), I suggest also adding a Toolkit Proxy (maybe with the adapter pattern) for the non-worker version to execute Verovio toolkit calls fake asynchronously (the promise becomes resolved immediately) to make the syntax is interchangeable, no matter whether you use a worker or not. I would suggest something like that:

Click to expand the code example

class Deferred {
    constructor() {
        this.promise = new Promise((resolve, reject) => {
            this.reject = reject;
            this.resolve = resolve;
        });
    }
}
createWorkerProxy() {
    this.tasks = {};
    this.worker = self.VerovioEnvironment.createWorker();
    this.worker.addEventListener('message', (event) => {
        const { id, result } = event.data;
        const deferred = this.tasks[id];
        if (deferred) {
            deferred.resolve(result);
            delete this.tasks[id];
        }
    });
    this.worker.addEventListener('error', (event) => {
        console.error(event);
    });
    return new Proxy(this, {
        get: (target, method) => {
            return (...args) => {
                const id = uuidv4();
                target.worker.postMessage({
                    id,
                    method,
                    args,
                });
                const deferred = new Deferred();
                this.tasks[id] = deferred;
                return deferred.promise;
            };
        },
    });
}
createNativeProxy() {
    moduleIsReady.promise.then(() => {
        this.verovioToolkit = new verovio.toolkit();
    });
    return new Proxy(this, {
        get: (target, method) => {
            return (...args) => {
                const deferred = new Deferred();
                if(method === 'onRuntimeInitialized') {
                    moduleIsReady.promise.then(() => {
                        deferred.resolve();
                    });
                } else {
                    const fn = this.verovioToolkit[method || null];
                    if(fn) {
                        deferred.resolve(fn.apply(this.verovioToolkit, args || []));
                    }
                }
                return deferred.promise;
            };
        },
    });
}

window.onunload event

Is the window.onunload event that destroys the wasm instance really needed? As far as I know, the browser does this itself. MDN also warns against using this event: https://developer.mozilla.org/en-US/docs/web/api/window/unload_event.

---

I have set up a repository to showcase some of these features: https://github.com/WolfgangDrescher/verovio-npm-improvements. It's still work in progress and not very mature yet, but for a first preview it can give a good insight I think. To try it out you can add "verovio": "github:WolfgangDrescher/verovio-npm-improvements", into the package.json dependencies. I can also provide a small example app in Vue.js if there is interest.

[WolfgangDrescher]

I just saw that this is also related to: #1908. Of course I can open a pull request, but these are major and breaking changes, so I think it's better to discuss about the integration first.

Also note that my demo (https://github.com/WolfgangDrescher/verovio-npm-improvements) seems to break currently. I get a JS error RuntimeError: memory access out of bounds that gets thrown in _vrvToolkit_loadData. First I thought that my wasm file is buggy since I got some errors during the Emscripten build. But then I built the toolkit with a GitHub workflow from ci_build.yml and got the same result. Has this already occurred somewhere else? Or did the new set up somehow messing this up?

[lpugin]

Thank you for looking into this. Let's move it to a discussion and then have issues for each topic separately whenever needed.