tools icon indicating copy to clipboard operation
tools copied to clipboard

refactor(rome_wasm): move the WASM bindings for the Workspace to a crate

Open leops opened this issue 3 years ago • 3 comments

Summary

This moves the WASM bindings to the Workspace API out of the playground into a dedicated rome_wasm crate. This crate is built with wasm-pack in the CLI release workflow to generate 3 npm packages: @rometools/wasm-bundler, @rometools/wasm-nodejs and @rometools/wasm-web, acting as wrappers for the WebAssembly module for different platforms (bundler imports the WASM blob using ESM, nodejs loads it with the node.js fs API, and browser loads it using the WHATWG fetch API)

The wasm-web package is now used to provide the playground features, although it makes the playground development workflow somewhat convoluted since it's necessary to run pnpm build:wasm (or pnpm build:wasm-dev) before running pnpm install to ensure the package has been previously generated on the file system.

I'm opening this as a draft for now since I'm not entirely sure this is the way we want to go with for the WASM packaging and distribution. Open questions are:

  • Does it make sense to have multiple "platform specific" packages or should we find a way to merge them into a single bundle that always does "the right thing" ?
  • Relatedly, should these packages be considered internal (like the @rometools/cli-* packages) and only be used through the rome frontend package, or should we advertise them to end user and let them pick the correct one for their platform ?

Test Plan

This is mostly a build infrastructure change, tested through the playground deployment workflow and nightly release workflow.

Besides that, the wasm-bindgen ecosystem supports building tests to WASM and running them in a headless browser with wasm-bindgen-test. We could perform some additional testing on the Workspace API there, but I'm not sure if it would provide much additional value or be mostly redundant with the existing CLI and LSP tests

leops avatar Aug 12 '22 13:08 leops

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 12839f7
Status: ✅  Deploy successful!
Preview URL: https://469098d3.tools-8rn.pages.dev
Branch Preview URL: https://feature-workspace-wasm.tools-8rn.pages.dev

View logs

  • Does it make sense to have multiple "platform specific" packages or should we find a way to merge them into a single bundle that always does "the right thing" ?

I think it makes sense to have platform specifics packages, for the time being. For the browser one, yes, it's definitely something that we have to. For Node.js, I would say YES for the time being, because it's one of the options we want to try and deliver the APIs. If one day we decide that we don't need it anymore, we can deprecate it.

  • Relatedly, should these packages be considered internal (like the @rometools/cli-* packages) and only be used through the rome frontend package, or should we advertise them to end user and let them pick the correct one for their platform ?

There are pros and cons. For the time being I think we should keep them private, at least we figure out how to use these packages in our internal rome frontend package. This would give us time to test them in different enviroments and situations, allowing us to fix stuff.

Then, once we know how to work with these, we might want to consider to promoting them and let the user decide.

ematipico avatar Aug 15 '22 15:08 ematipico

do we plan to set a publishing workflow of this packages in another PR?

The publishing workflow is already included in this PR, the release_cli workflow automatically publishes all packages under the npm directory so I simply extended it to also build the WASM distribution

do we plan to publish @rometools/wasm-web and use it directly in our playground using another PR?

That's something we may do in the future, but at the moment I think the Workspace API is not stable enough to allow this (we need to push changes to the playground in sync with every modification we do on the workspace), so I'm importing the wasm-web package into the playground using a direct path reference for the time being

do you plan to use pnpm workspace to import @rometools/wasm-web in another PR?

I'll probably try to have a single pnpm workspace at the root of the repo eventually like we do for Cargo, I just need to come up with a good list of what package should be included in it and what package should not (out of the vscode extension, various npm packages, website and playground). And also figure out if having a bunch of auto-generated packages that may or may not exist is handled correctly

leops avatar Aug 18 '22 08:08 leops

The publishing workflow is already included in this PR, the release_cli workflow automatically publishes all packages under the npm directory so I simply extended it to also build the WASM distribution

Why the workflow it's together with the CLI? If there are errors in these packages (even though the source code is slim), should we not be able to publish them also independently ? Not a blocker for this PR

ematipico avatar Aug 18 '22 09:08 ematipico

The publishing workflow is already included in this PR, the release_cli workflow automatically publishes all packages under the npm directory so I simply extended it to also build the WASM distribution

Why the workflow it's together with the CLI? If there are errors in these packages (even though the source code is slim), should we not be able to publish them also independently ? Not a blocker for this PR

This is because the CLI is published to npm under the same rome package we'll also eventually use for the node.js API, since all the packages under npm/ will depend on each other they need to be built in the same workflow to ensure they stay in sync (we should probably rename that workflow from release_cli to release_npm to make it clearer)

leops avatar Aug 18 '22 09:08 leops