+
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

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

Merged
merged 3 commits into from
Aug 18, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Aug 12, 2022

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.

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 leops temporarily deployed to aws August 12, 2022 13:32 Inactive
@github-actions
Copy link

github-actions bot commented Aug 12, 2022

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Aug 12, 2022

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

@leops leops temporarily deployed to aws August 12, 2022 13:40 Inactive
@ematipico
Copy link
Contributor

  • 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.

@leops leops force-pushed the feature/workspace-wasm branch from 891e9dc to 906377f Compare August 18, 2022 07:14
@leops leops temporarily deployed to aws August 18, 2022 07:14 Inactive
@leops leops temporarily deployed to aws August 18, 2022 07:27 Inactive
@leops leops marked this pull request as ready for review August 18, 2022 07:36
@leops leops requested a review from a team August 18, 2022 07:36
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few questions:

  • do we plan to set a publishing workflow of this packages in another PR?
  • do we plan to publish @rometools/wasm-web and use it directly in our playground using another PR?
  • do you plan to use pnpm workspace to import @rometools/wasm-web in another PR?

@leops
Copy link
Contributor Author

leops commented Aug 18, 2022

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

@ematipico
Copy link
Contributor

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

@leops
Copy link
Contributor Author

leops commented Aug 18, 2022

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 leops merged commit 02df267 into main Aug 18, 2022
@leops leops deleted the feature/workspace-wasm branch August 18, 2022 14:38
IWANABETHATGUY pushed a commit to IWANABETHATGUY/tools that referenced this pull request Aug 22, 2022
…ate (rome#3056)

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

* fix pull_request_js workflow

* fix CI
@ematipico ematipico linked an issue Aug 23, 2022 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create a new npm library to bundle webassembly code
2 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载