-
Notifications
You must be signed in to change notification settings - Fork 593
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Tyler Cloutier <cloutiertyler@users.noreply.github.com>
cab134a
to
36c204b
Compare
36c204b
to
f032caf
Compare
# 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>
There was a problem hiding this 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.
There was a problem hiding this 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.
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 forAlgebraicType
andRawModuleDef
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: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 theAlgebraicType
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 useAlgebraicType
directly.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.ProductValue
type has been removed.ReducerArgsAdapter
andValueAdapter
types, which were used with AlgebraicValues have been removed.Listed below are the non-breaking API changes:
T
, I am now exporting aTVariants
namespace which has the types of all the variants forT
. This was previously exposed on the namespaceT
, but was inaccessible in modules other than the one it was defined in because I also export a typeT
in addition to namespaceT
and in the type positionT
was being interpreted as a type rather than a namespace. It's unclear why TypeScript resolved it as theT
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 inTVariants
. (Open to other names for this namespace).Expected complexity level and risk
Testing
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.