-
Notifications
You must be signed in to change notification settings - Fork 667
Refactors TypeScript into a single spacetimedb package
#3248
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
Conversation
JulienLavocat
left a comment
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.
LGTM, I really like the syntax!
06420b3 to
5403b47
Compare
spacetimedb package
|
This seems fine to me, but likely breaks the current deploy process since we only mirror This may have to come hand-in-hand with migrating over the npm+provenance stuff that I've been putting off migrating. |
|
@bfops Mm, that is a good point, although if we add publishing |
JulienLavocat
left a comment
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.
That was a lot of work! LGTM, I've left one minor comment that you can ignore if you wnat
|
@cloutiertyler @bfops I think we should only publish the |
Description of Changes
This PR moves most of the contents of
@clockworklabs/spacetimedb-sdkinto thespacetimedbmodule. Thespacetimedbmodule now exportssdkandserveras separate subpaths wheresdkcontains the code which was previously in@clockworklabs/spacetimedb-sdk.In particular it makes the following moves:
/sdks/typescript/packages/sdk->/sdks/typescript/sdks/typescript/packages/sdk->crates/bindings-typescript/sdks/typescript/packages/test-app->crates/bindings-typescript/test-appThe following packags was NOT moved:
/sdks/typescript/examples/quickstart-chatMotivation
In accordance with #3250, we would like to consolidate
@clockworklabs/spacetimedb-sdkinto a singlespacetimedbpackage so that users can import the different things they need from a single package.Pros:
spacetimedb,spacetimedb/react,spacetimedb/sdk,spacetimedb/server, etc.@clockworklabs/spacetimedb-sdkwhich now becomes a thin wrapperspacetimedbpackage into other packages if we want to split them up (e.g.@spacetimedb/lib,@spacetimedb/sdk, etc.) and we can solve the build complexity that introduces when we get to itbindings-csharpout of the crates directory where it probably doesn't belong anyway/sdks/typescriptif we wanted to leave that separateCons:
sdkdirectory is now a bit of a ruse as to where the code actually lives since it's just a thin wrapper. If it eventually becomes its own independent package, we'll also have to break up spacetimedb into@spacetimedb/liband@spacetimedb/serverso that@clockworklabs/spacetimedb-sdkcan depend on@spacetimedb/libwhile being a dependency ofspacetimedb.Ideally this change would have been made later, however, it became necessary for the following heinously disastrous chain of forcing moves:
reactsupport necessitated shipping react as an optional peer dependency under@clockworklabs/spacetimedb-sdk/react@clockworklabs/spacetimedb-sdkwas configured to havenoExternalforspacetimedbmeaning it would collect the library into the sdk bundle. I attempted to continue this for react support but...reactbundle which also pulled inspacetimedbcaused their to be nominal type conflicts between classes in the duplicatespacetimedbbundles.spacetimedbto be included asexternalcaused compile errors because@clockworklabs/spacetimedb-sdkis configured intsconfig.jsonto fail on unused variables and it was now including the source ofspacetimedbwhich is not configured to error on unused variables and has "unused" private variables which are actually used by us secretly, but not exposed to the clients.spacetimedbto also be published to npm as its own module which@clockworklabs/spacetimedb-sdknow imports, which requires that we addtsupconfig tospacetimedbto publish a built version of the library.sdkandreactcode from@clockworklabs/spacetimedb-sdkinto the existingspacetimedbpackage to avoid the duplicate import problem on step 4 and change@clockworklabs/spacetimedb-sdkback to again usenoExternalfor itsspacetimedbdependency.And here we are. I chose not to move
/crates/bindings-typescripteven though that's probably not a great place long term. It would be better to have it in/packages/spacetimedbor/npm-packages/spacetimedbor/ts-packages/spacetimedbor something, and move all our TypeScript packages in there. But that is a different matter.The net result however is that we have a new
spacetimedbpackage which exports the different parts of the API under:spacetimedbspacetimedb/serverspacetimedb/sdkspacetimedb/reactwhile still not breaking the existing deploy process, nor any users/developers who are currently using
@clockworklabs/spacetimedb-sdk.I think long term should we ever decide to split
spacetimedbup into multiple packages or if we have additional unrelated packages, we should publish them to the@spacetimedborg which I reserved for us here:https://www.npmjs.com/org/spacetimedb
API and ABI breaking changes
This should not change or modify the API or ABI in any way. If it does so accidentally it is a bug, although I carefully went through the exports.
Expected complexity level and risk
3 because it changes how the SDK is built a bit and rearranges a lot of paths.
Testing