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

Unify Type System packages #808

Merged
merged 21 commits into from Dec 2, 2022
Merged

Unify Type System packages #808

merged 21 commits into from Dec 2, 2022

Conversation

Alfred-Mountfield
Copy link
Contributor

@Alfred-Mountfield Alfred-Mountfield commented Dec 1, 2022

🌟 What is the purpose of this PR?

Background

Due to quirks of WASM, the Type System package was split into 2 variations, one for use in node environments, and one in web. This was a temporary solution, as it was one we knew would cause problems in the long one. The main pain-point of this approach is when wanting to use the package in a multi-environment setup, such as Next.js which ideally wants a single package it can use in server-side and client-side code. We tried various attempts to unify the packages, defining multiple entrypoints, combining the various outputs of wasm-pack targets (web, ESM, etc.)

We have settled on this solution for now, largely inspired by the excellent advice of Nick Babcock, and libraries written by him, or linked to by him:

In the future we may move to an ESM only approach, as this removes the need for an initialization step due to its integrated support for async imports, but to ensure the package is widely-usable at the moment, we have opted to produce various entrypoints that can be used depending on environment and needs.

The unified package

The new package is generated by taking the web target of wasm-pack and performing another bundling step to produce a common package for (hopefully) all popular JS-based environments. The step is performed by rollup for similar reasons to those outlined in Nick's post. The bundling process creates a number of targets, which are routed to through the exports field of the package.json (please consult the source-code for more context).

This does mean that all targets now require an async initialization step, whereas previously only the web ones did. (Please see Known Issues below for more information).

As stated, the package has various entry points depending on the consuming environment. These have two different flavours, a fat, and a slim variant. Both variants allow the consumer to pass in a compiled WASM module should they wish (this is important for browser caching benefits and other things). The slim variants specifically do not include inlined versions of the WASM, and therefore are much smaller, but also require the user to acquire a copy of the WASM source from somewhere else. This should allow a single WASM source to be bundled in a complex application (such as an embedding application with many blocks), or for us to host the binary somewhere accessible.

There are a lot of subtleties and reasons for this, which again, are outlined in depth in Nick's post.

πŸ”— Related links

🚫 Blocked by

  • Updating the changeset
    • Shouldn't be necessary for a new package
  • Deciding on if we should yank/deprecate/something the existing partitioned packages
    • We'll deprecate the old ones through the CLI later on
  • Setup a flow on how to publish the library
    • Because it builds to dist and uses the same yarn command layout as other packages, we expect distribution to just work once merged.

πŸ” What does this change?

  • Introduces a new @blockprotocol/type-system package
  • Migrates the type-system crate build logic to produce a package in the new location
  • Adds a rollup config to inline the WASM as a string, while also producing slim bundles
  • Moves the jest tests over and modify them to appropriately initialize the WASM

πŸ“œ Does this require a change to the docs?

  • The Type System is part of work-in-progress changes to the Block Protocol graph service at the moment. When these changes are stabilised, documentation should be updated to point to its usage.

⚠️ Known issues

  • The WASM package is generating a rather large binary file at the moment (~1MB for the binary and ~1.5MB for the encoded string), this is somewhat mitigated by the fact that we are producing a slim build. This should mean that for blocks, or other implementations that might care about package size, we should be able to host the WASM somewhere (or they can), and import it themselves at runtime with fetch.
  • Using WASM (almost always) involves an async step. It's necessary for the browser to compile the binary into a WASM module, and this happens asynchronously. With ESM this was abstracted away, but for the unified package it's necessary at the moment to have a consistent API. This means that all of the packages now involve calling an init function. Not just the browser based ones.
    • It is possible that this can be modified slightly without fully migrating to ESM. In the ES target (which we still produce) it should be possible to have a different source index file which calls the initialize logic before exporting the rest of the code. During testing I found this produced issues when the target was imported into environments such as Next.js, which mangles the import name of the WASM asset and moves its location. This could perhaps be rectified by always having the WASM be inlined in this case, or dedicated resources to produce configs for bundler processes such as Next.js. Unfortunately producing these would be a lot of work, the previous solution we discovered did not work with the new bundle in my testing.
    • Secondly, removing the need for initialization in ESM modules isn't necessarily a big win. ESM has access to top-level awaits so it's not a lot of overhead to call initialize, and having inconsistent API exports will be deeply annoying and/or make using the package broken in places like Next.js if the browser import is non-ESM.
  • Doing an import wasm from "<module>.wasm" to use in the slim variants causes problems when trying to compile it. Slim variants are still important and usable if you pass a URL into the initialize method, rather than trying to import a WASM object, this has been tested by putting the WASM asset into the public folder of a Next.js app. The error messages are can't resolve "wbg", which there doesn't seem to be a lot of content about online. We have a theory that this is to do with the wasm no-longer sitting alongside its respective JS, and perhaps needs to import from JS or something, but that will require deeper investigation. For now, using embedded strings is more convenient anyway, and as such this can be addressed in the future. Note, this issue only seems to occur when the functions are dealing with JsValue inputs, (passing to and from JS) which we make heavy use of.

🐾 Next steps

  • Shrink the WASM binary size
  • Expand the helper function collection
  • Improve validation of types

πŸ›‘ What tests cover this?

  • There are jest tests included within the WASM package. We may want to consider introducing the following environments accompanied by something like Playwright to ensure it's adequately tested:
    • a test block
    • a test Next.JS app
    • a ts-node script

❓ How to test this?

  1. Please follow the README, it should be a good test to ensure the instructions are adequate

@github-actions github-actions bot added area: crates area: dependencies Relates to third-party or otherwise imported dependencies (area) area: infra Relates to version control, CI, CD or IaC (area) area: libs > @Þ/type-system Affects `@blockprotocol/type-system-node` or `@blockprotocol/type-system-web` (library) labels Dec 1, 2022
@kachkaev
Copy link
Contributor

kachkaev commented Dec 1, 2022

Possibly replaces #652

@codecov
Copy link

codecov bot commented Dec 1, 2022

Codecov Report

Merging #808 (cd85a39) into main (4a15864) will not change coverage.
The diff coverage is n/a.

❗ Current head cd85a39 differs from pull request most recent head a4919ad. Consider uploading reports for the commit a4919ad to get more accurate results

@@           Coverage Diff           @@
##             main     #808   +/-   ##
=======================================
  Coverage   62.56%   62.56%           
=======================================
  Files         271      271           
  Lines        4266     4266           
  Branches     1004     1004           
=======================================
  Hits         2669     2669           
  Misses       1257     1257           
  Partials      340      340           
Flag Coverage Ξ”
site-integration-chrome 61.48% <ΓΈ> (-0.03%) ⬇️
site-integration-firefox 61.53% <ΓΈ> (ΓΈ)
site-integration-iphone 57.21% <ΓΈ> (ΓΈ)
site-integration-pixel 59.44% <ΓΈ> (+0.44%) ⬆️
site-integration-safari 57.71% <ΓΈ> (ΓΈ)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

packages/@blockprotocol/type-system/src/index.ts Outdated Show resolved Hide resolved
.prettierignore Show resolved Hide resolved
with:
workspaces: crates/type-system

- name: Install tools
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Install tools
- name: Install Rust tools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to be even more specific

packages/@blockprotocol/type-system/README.md Outdated Show resolved Hide resolved
packages/@blockprotocol/type-system/README.md Outdated Show resolved Hide resolved
packages/@blockprotocol/type-system/package.json Outdated Show resolved Hide resolved
packages/@blockprotocol/type-system/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@kachkaev kachkaev left a comment

Choose a reason for hiding this comment

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

Thanks Alfie!

It’s a bit of a shame that CI checks are taking 1.5-2 mins longer because of Rust, but we can see how to address this separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dependencies Relates to third-party or otherwise imported dependencies (area) area: infra Relates to version control, CI, CD or IaC (area) area: libs > @Þ/type-system Affects `@blockprotocol/type-system-node` or `@blockprotocol/type-system-web` (library)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants