Skip to content

Separate out TypeScript module library from the SDK #3182

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

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

cloutiertyler
Copy link
Contributor

@cloutiertyler cloutiertyler commented Aug 19, 2025

Description of Changes

Please note, much of the code changed in this PR is generated code.

This change updates the TypeScript SDK to use a new spacetimedb TypeScript library which lives under the /crates/bindings-typescript folder alongside /crates/bindings-csharp. Just like with the C# bindings library, the types for AlgebraicType and RawModuleDef are now code generated with a script in /crates/codegen/examples.

Pulling this out into a library allows us to use the same types and serialization code on both the server and the client for TypeScript modules.

In the process of making this change I also found and fixed several issues with the TypeScript code generation. Namely an issue with recursive types and an issue with the never type.

I have also improved the npm/pnpm scripts to be able to generate TypeScript code for us automatically by running pnpm generate for any place that we have to generate typescript code. Namely:

  • Quickstart module bindings
  • AlgebraicType/ModuleDef for TypeScript module library
  • Client API messages for the TypeScript API
  • TestApp module bindings

API and ABI breaking changes

IMPORTANT! This is an API breaking change for clients, as such it should be a major version change. However, I am going to see if I can shim in the old API as best as possible to make it compatible.

Notably, we were previously exporting APIs that end users do not need, and I don't think it would ever occur to them to use, namely the whole AlgebraicValue API and also the AlgebraicType API. In principle, no one should have a need for these, although it was technically possible for them to use it.

Indeed, we could potentially even just remove AlgebraicType completely from the API by directly code generating the serialization code.

Listed below are all of the BREAKING changes to the API and their effect:

  • AlgebraicType is now a structural union literal type instead of a class (nominal type), this is a consequence of generating the type with our code gen. Users did not have a reason to use AlgebraicType directly.
  • The AlgebraicValue type has been removed entirely. This was previously a class that was exported from the SDK, but very unlikely to be used by users.
  • The ProductValue type has been removed.
  • The ReducerArgsAdapter and ValueAdapter types, which were used with AlgebraicValues have been removed.
  • Generated code has changed incompatibly so users will have to regenerate code when upgrading to this version. Technically a breaking change, but pretty easy to fix.

Listed below are the non-breaking API changes:

  • I am now exporting generated product types as the default export in addition to how I was exporting them before
  • For generated sum type T, I am now exporting a TVariants namespace which has the types of all the variants for T. This was previously exposed on the namespace T, but was inaccessible in modules other than the one it was defined in because I also export a type T in addition to namespace T and in the type position T was being interpreted as a type rather than a namespace. It's unclear why TypeScript resolved it as the T namespace within the module in which is was defined previously. Anyway, since the old types were apparently unobservable to users, I've replaced them with the types in TVariants. (Open to other names for this namespace).
  • I fixed a bug where never types (sum types with no variants) were not correctly generated.

Expected complexity level and risk

  1. The most complex thing about the PR are the potential impacts to the API. I am reasonably certain, but not 100% certain that I have accounted for everything above.

Testing

  • I ran pnpm test which runs all the tests in the repo including an integration test which tests the connection to SpacetimeDB. I also fixed broken tests.

@cloutiertyler cloutiertyler changed the title Tyler/ts module test Separate out TypeScript module library from the SDK Aug 20, 2025
@bfops bfops force-pushed the tyler/ts-module-test branch from cab134a to 36c204b Compare August 20, 2025 22:07
@bfops bfops force-pushed the tyler/ts-module-test branch from 36c204b to f032caf Compare August 20, 2025 22:08
github-merge-queue bot pushed a commit that referenced this pull request Aug 21, 2025
# Description of Changes

We had weird caching issues in the C#/Unity testsuite. Somehow, they got
triggered only as of
#3181 merging, and I
have no idea why/how.

I've restored the `id` field of the checkout step (which is used by the
cache step), and this _seems_ to have fixed it.

# API and ABI breaking changes

None.

# Expected complexity level and risk

1

# Testing

- [x] It passes on this PR
- [x] It passes in a test PR that combines this change with
#3182

---------

Co-authored-by: Zeke Foppa <bfops@users.noreply.github.com>
@cloutiertyler cloutiertyler marked this pull request as ready for review August 21, 2025 20:34
@cloutiertyler cloutiertyler requested review from JulienLavocat, gefjon and jsdt and removed request for JulienLavocat August 21, 2025 20:37
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

I have no objections to the described breaking changes. I think that ideally we should save them for a minor version bump and include them in the release notes, under a heading like "Minor incompatible changes" or something. But I don't see any reason to hold them for a major version bump other than fanatical devotion to the SemVer spec.

I don't otherwise feel equipped to review this PR, and would prefer to leave it to others.

Copy link
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

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

the .prettierignore file should not be moved without changes. The previous file was relative to the directory it was in. We should probably at least change /.github to .github in there.

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