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

Improve JavaScript build and NPM version #1315

Closed
wants to merge 29 commits into from

Conversation

WolfgangDrescher
Copy link
Contributor

  • Refactor import of verovio toolkit and emscripten module
  • VerovioToolkitProxy: Wrapper class as a JS-Proxy with magic function that calls the methods on the toolkit in th worker
  • Use webpack to bundle all scripts into a JS file that can be run as a normal JS file, as a Node-Module or a Worker
  • Make ./buildToolkit -H -w work (BINARYEN_ROOT must be set up in .emscripten)
  • Test if built dist/index.js will work as a default toolkit but also as a worker
  • Review and improve new commands in ./buildToolkit
  • Write docs on how to implement verovio as a worker

@lpugin
Copy link
Contributor

lpugin commented Feb 20, 2020

Please make sure you are not trying to achieve to many things in one go. I am not sure what you need to change in buildToolkit in order to make it work since it seems to work properly. Make a separate PR for this.

Regarding the VerovioEmscriptenAdapter.js, we certainly do not want another file that wraps all the methods. Same for the VerovioToolkit.js one. They look pretty redundant with the verovio-proxy.js functions - expect there is no class, but I don't think this is really the issue. Is it? Duplicating all this will not be good for maintenance.

@WolfgangDrescher
Copy link
Contributor Author

I don't think another PR is necessary since the build process would not change a lot. I just added another layer with webpack after builtToolkit has created the Module to make a browser version out of it (that can also be used in a node environment). See me changes in buildToolkit: (cd $NPM && npm install), (cd $NPM && npm run build).
I'm currently just having issues building the toolkit with the -w option.

Are your travis pipelines also building the npm version? If so: do they also use buildToolkit?

My intention is not to add another file with VerovioEmscriptenAdapter.js and VerovioToolkit.js but to replace and relocate them to make them reusable (e.g. the VerovioToolkitProxy needs the toolkit so we just need to import it there). Because i did not want to change to much of the existing code i did not merge VerovioEmscriptenAdapter directly into the VerovioToolkit and let them as they are but split them into two files.

In my opinion the buildToolkit should not do much more than building a JS file with the Module and adding an export statement at the end. Currently verovio-npm-start.js, verovio-proxy.js, etc. are concatenated in buildToolkit. I think bundling a runnable JS version with all its dependant classes like the toolkit, proxy, worker and helpers should be done with webpack.

Currently buildToolkit is building two JS files if i understand it correctly. One for general JS usage and one for npm. As far as i can see the only significant difference is the module.exports at the end of the npm version. Using webpack to bundle everything together it will not be necessary to have two different files anymore, since webpack adds a build factory at the beginning it will only export the modules if this is possible, otherwise it will assign them to window.

Am i correct that the wohle npm directory is shipped as an npm package? My modifications will build the JS into ~/emscripten/npm/dist/index.js.

@lpugin
Copy link
Contributor

lpugin commented Feb 20, 2020

I'm currently just having issues building the toolkit with the -w option.

Do you have the emdk installed? If yes, what do you get with emcc --version

Are your travis pipelines also building the npm version? If so: do they also use buildToolkit?

No, the npm package is build only for releases, where a bunch of scripts are run locally. This includes npm publish at some point.

Not everybody uses node and webpack, so we do have to keep buildToolkit generating a toolkit that bundles the Module and the verovio-proxy.js. If you need to have the 'raw' wasm output to build a module, just use the verovio.js produced by buildToolkit. I think this is what you need.

From there you can make a separate script, which might be simpler to start with. Once everything runs we can see if we can to leave it like this or merge it into buildToolkit.

@WolfgangDrescher
Copy link
Contributor Author

If yes, what do you get with emcc --version

emcc (Emscripten gcc/clang-like replacement) 1.38.44 ((unknown revision))
Copyright (C) 2014 the Emscripten authors (see AUTHORS.txt)
This is free and open source software under the MIT license.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I tried installing emsdk like in your travis files like @musicEnfanthen quoted out in #1253 (comment) but it seems like there are issues with a SSL certificate when i run ./emsdk install latest.

This includes npm publish at some point.

I see. Then i keep working directly in the npm directroy.

Not everybody uses node and webpack

Thats exactly why i am using it. Webpack will build me a plain JavaScript version that can be used as a standalone version. At the same time it should be possible to use the same file if you want to import it in node. Like this we have the advantage that during development we can benefit from node imports. Webpack will only be used in the build process and is not needed for users of the verovio package.

keep buildToolkit generating a toolkit that bundles the Module and the verovio-proxy.js.

That basically what i am achieving to do. But with webpack instead.

From there you can make a separate script, which might be simpler to start with. Once everything runs we can see if we can to leave it like this or merge it into buildToolkit.

Sounds good. So i will revert my changes in buildToolkit and add a new script (e.g. bundleToolkit) for building the toolkit with webpack.

@musicEnfanthen
Copy link
Contributor

there are issues with a SSL certificate when i run ./emsdk install latest.

Maybe related? emscripten-core/emscripten#6723: ./emsdk install latest : Installation failed

Does it work with python ./emsdk install latest ?

@lpugin
Copy link
Contributor

lpugin commented Jun 4, 2020

Please open an updated PR when appropriate.

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

Successfully merging this pull request may close these issues.

3 participants