Add TypeScript submodule support for SpacetimeDB modules#5486
Conversation
80e8b25 to
5de4e3d
Compare
5de4e3d to
84c80c6
Compare
| } | ||
| } | ||
|
|
||
| type FlatSubmoduleDispatch = { |
There was a problem hiding this comment.
Same "Dispatch" naming question
There was a problem hiding this comment.
Similar answer to above
|
|
||
| (anon ? ctx.anonViews : ctx.views).push({ | ||
| fn, | ||
| fn: fn as unknown as ViewFn<any, any, any>, |
There was a problem hiding this comment.
This type cast is pretty sus. Why is this necessary?
There was a problem hiding this comment.
a lot of this was with help of claude this was the reason it gave for this:
`fn`'s static type is the conditional `Anonymous extends true ? AnonymousViewFn : ViewFn`,
but `Anonymous` is only resolved at runtime via `anon`. TypeScript can't correlate the two,
so the cast bridges a distinction the type system can't express here; `anon` picks the
matching array at runtime, and view dispatch (in `runtime.ts`) invokes `fn` with the ctx
shape appropriate to that array, regardless of the type recorded when it was pushed.
There was a problem hiding this comment.
This really smells like LLM BS justification to me unfortunately. Or rather it is literally true, but does not rule out another way to do it that would not require this cast.
| &mut self, | ||
| num: Option<u32>, | ||
| module_def: &RawModuleDefV9, | ||
| module_def: &ModuleDef, |
There was a problem hiding this comment.
Can you explain why these are all changing?
There was a problem hiding this comment.
ModuleDefV9 doesn't have submodules so any submodule related subscription (e.g.spacetime subscribe select * from lib.table would fail as the command wouldn't be able to know the type of table as it would only know about tables in the root module)
There was a problem hiding this comment.
I am medium concerned that this might be a breaking change of some kind. I know that for example there are places that we read in a V9 or V10 raw version and parse it. I'd approve but I'll leave this comment open. When you verify that it's not a breaking change, you can resolve the conversation and merge it.
There was a problem hiding this comment.
I haven't taken the time to understand all this, but before I do, I just wanted to relay the goal that the generated code should be the same as what you write in the module itself. Does this new codegen maintain that property, at least largely?
There was a problem hiding this comment.
I am not sure I follow exactly, the generated code is the same as what is generated for root modules however to avoid naming conflicts submodule types are generated inside subfolders. This is the main different between code generation for root types and code generation for submodule types.
There was a problem hiding this comment.
I'm saying that we have a requirement that we want users to be able to import their module code into the client to be able to use them as types. Obviously if their module is written in a different language we cannot do that so we have to generate client bindings. I'm saying the client bindings should/must match what the user would have written in their TypeScript module if they had written one. And we should avoid diverging from that principle.
| pub table_id: TableId, | ||
| pub view_def: &'a ViewDef, | ||
| /// The full namespaced view name as stored in `st_view` (e.g. `"lib.library_view"`). | ||
| pub view_name: RawIdentifier, |
There was a problem hiding this comment.
RawIdentifier is not a great name for a dot delimited identifier or whatever. It implies that I could convert it into an Identifier by validating it, and I don't think that's possible.
There was a problem hiding this comment.
These are now using NamespacedIdentifier which is a list of Identifier
There was a problem hiding this comment.
I think I'd like to have @joshua-spacetime look at the view changes.
|
|
||
| if *is_anonymous { | ||
| if view_instances.is_empty() { | ||
| if !subs.iter().any(|a| matches!(a, ViewInstanceArgs::Anonymous)) { |
There was a problem hiding this comment.
This seems materially different.
There was a problem hiding this comment.
Restored the is_empty() check
|
|
||
| pub trait InstanceOp { | ||
| fn name(&self) -> &Identifier; | ||
| fn name(&self) -> &RawIdentifier; |
There was a problem hiding this comment.
RawIdentifier really feels like the wrong name for a multipart identifier.
There was a problem hiding this comment.
This is now using &str. name here isn't meant to be a validated name, it's only used for logging. However I am going to see if it can be made into a NamespacedIdentifier
There was a problem hiding this comment.
I couldn't, but I think &str makes sense here given how it's used (logging)
cloutiertyler
left a comment
cloutiertyler
left a comment
There was a problem hiding this comment.
This definitely needs some cleanup. The most notable things that need clean up are uses of RawIdentifier and NamespacedIdentifier::new_assume_valid after validating the module def.
Left comments for the other stuff.
|
|
||
| for view in module_def.views() { | ||
| if view_backing_table_needs_recreate(stdb, tx, module_def, view)? { | ||
| let name = NamespacedIdentifier::new_assume_valid(view.name.to_string()); |
There was a problem hiding this comment.
We should not be passing non-namespaced identifiers into here and doing new_assume_valid. TBH we should not have any calls like this of any kind.
There was a problem hiding this comment.
This is now creates a NamespacedIdentifier from the view name which is also a NamespacedIdentifier
| spacetimedb_schema::auto_migrate::AutoMigrateStep::AddRowLevelSecurity(sql_rls) => { | ||
| log!(logger, "Adding row-level security `{sql_rls}`"); | ||
| let rls = plan.new.lookup_expect(sql_rls); | ||
| let rls = plan |
There was a problem hiding this comment.
Shouldn't lookup_expect just support this?
There was a problem hiding this comment.
Replaced with the moduledef lookup function (which uses a hashmap so doesn't need to scan). lookup_expect panics if the key doesn't exist. I think it's probably ok to use the panicky version but this is slightly safer
| } | ||
|
|
||
| #[test] | ||
| fn stale_view_backing_schema_generates_startup_repair_plan() -> anyhow::Result<()> { |
There was a problem hiding this comment.
Why are we removing tests?
| /// is used to sort steps of an auto-migration so that removes precede adds for the same name. | ||
| #[derive(PartialEq, Eq, Debug, PartialOrd, Ord)] | ||
| pub enum AutoMigrateStep<'def> { | ||
| // It is important FOR CORRECTNESS that `Remove` variants are declared before `Add` variants in this enum! |
There was a problem hiding this comment.
Why are all of these comments being removed? This seems not good.
| /// Validated at execution time: fails if the table contains data. | ||
| RemoveTable(<TableDef as ModuleDefLookup>::Key<'def>), | ||
| /// Payload is the full namespaced table name (e.g., `"lib.library_table"` or `"user"`). | ||
| RemoveTable(NamespacedIdentifier), |
There was a problem hiding this comment.
This seems like a relaxation of type constraints, no?
There was a problem hiding this comment.
It is but <TableDef as ModuleDefLookup>::Key<'def> is of type Identifier which can't hold namespaced names
| assert!( | ||
| steps.contains(&AutoMigrateStep::RemoveSequence(&apples_sequence)), | ||
| steps.contains(&AutoMigrateStep::RemoveSequence( | ||
| NamespacedIdentifier::new_assume_valid("Apples_id_seq") |
There was a problem hiding this comment.
Please get rid of all these new_assume_valids
There was a problem hiding this comment.
these are now all using NamespacedIdentifiers built with valid Identifiers
| } | ||
|
|
||
| fn format_remove_table(&mut self, table_name: &Identifier) -> io::Result<()> { | ||
| fn format_remove_table(&mut self, table_name: &str) -> io::Result<()> { |
There was a problem hiding this comment.
Should be NamespacedIdentifier
| impl_deserialize!([] Identifier, de => RawIdentifier::deserialize(de).map(Self::new_assume_valid)); | ||
|
|
||
| /// Validates that `name` is a valid identifier string | ||
| pub fn validate_identifier(name: &str) -> Result<(), IdentifierError> { |
There was a problem hiding this comment.
Why does this need to be a separate function? I would assume a NamespacedIdentifier would be a list of Identifiers
| /// Root-level items use their plain name with no separator (e.g., `"user"`). | ||
| /// Construction from known-valid components should use `new_assume_valid`. | ||
| #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| pub struct NamespacedIdentifier(Box<str>); |
There was a problem hiding this comment.
This should be a list (or pair) of Identifier not a str.
There was a problem hiding this comment.
NamespacedIdentifier is now a list of Identifier
| /// The name of a table. | ||
| #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| pub struct TableName(Identifier); | ||
| pub struct TableName(RawIdentifier); |
There was a problem hiding this comment.
This should definitely not be a RawIdentifier
There was a problem hiding this comment.
This is now changed to be a NamespacedIdentifier instead
- TypeScript server SDK: runtime API for module mounting (submodules), including namespaced dispatch, ctx.as helpers for cross-namespace calls, and HTTP handler delegation - TypeScript client codegen: namespace-aware table/reducer/procedure exports and query builder for submodule tables and views - Migrations: auto-migrate support for submodule tables, views, and indexes; AddConstraint step added alongside NamespacedIdentifier-based migration steps - CLI: namespace support for `spacetime call` and `spacetime sql` - C++/C# bindings: autogenerated RawSubmoduleV10 types - module-test-ts: submodule integration test (lib_submodule)
- constraint and sequence names didn't have appropriate namespaced prefix - root tables should match def name exactly - sql-parser didn't accept namespaced column in WHERE and JOIN clauses
153ce36 to
d3a7a4f
Compare
| const libSubmoduleSchema = schema({ lib_data }); | ||
| export default libSubmoduleSchema; | ||
|
|
||
| export const lib_insert = libSubmoduleSchema.reducer( |
There was a problem hiding this comment.
This should really be camelCase.
| ); | ||
|
|
||
| // use_submodule_procedure: calls the lib submodule's lib_count procedure and returns the result. | ||
| export const use_submodule_procedure = spacetimedb.procedure( |
There was a problem hiding this comment.
Never use snake_case in TypeScript. Always camelCase.
| import { Router } from 'spacetimedb/server'; | ||
|
|
||
| // delegate the /health route to the submodule's handler | ||
| export const health_check = spacetimedb.httpHandler((ctx, req) => { |
There was a problem hiding this comment.
Needs to be camelCase.
| ### CLI | ||
|
|
||
| ```bash | ||
| spacetime call my-database "myauth/verify_token" '{"token": "abc123"}' |
There was a problem hiding this comment.
Should be camelCase.
| (ctx, { token }) => { /* ... */ } | ||
| ); | ||
|
|
||
| export const session_count = spacetimedb.procedure( |
There was a problem hiding this comment.
Should be camelCase, never use snake_case in TS.
| /// Returns the handle used by reducers to read from `args` | ||
| /// as well as the handle used to write the error message, if any. | ||
| fn start_funcall(&mut self, name: Identifier, ts: Timestamp, func_type: FuncCallType) { | ||
| fn start_funcall(&mut self, name: RawIdentifier, ts: Timestamp, func_type: FuncCallType) { |
There was a problem hiding this comment.
This should not be RawIdentifier.
| // Start the timer. | ||
| // We'd like this tightly around `call`. | ||
| env.start_funcall(op.name().clone(), op.timestamp(), op.call_type()); | ||
| env.start_funcall(RawIdentifier::new(op.name()), op.timestamp(), op.call_type()); |
There was a problem hiding this comment.
This should also not be raw identifier.
|
|
||
| pub trait InstanceOp { | ||
| fn name(&self) -> &Identifier; | ||
| fn name(&self) -> &str; |
There was a problem hiding this comment.
This should not be a raw str
| impl InstanceOp for ReducerOp<'_> { | ||
| fn name(&self) -> &Identifier { | ||
| self.name.as_identifier() | ||
| fn name(&self) -> &str { |
There was a problem hiding this comment.
All these changes from Identifier to str should be fixed. We should be using Identifier if it's an Identifier or NamespacedIdentifier if it is a NamespacedIdentifier, but never raw strs.
2 participants
Add TypeScript submodule support for SpacetimeDB modules
Description of Changes
spacetime callandspacetime sqlAPI and ABI breaking changes
Adds a submodule field to ModuleDef. Older modules will be publishable to new servers, but new modules that have submodules will not be publishable to old servers.
Expected complexity level and risk
5
Testing
Beyond the rust tests defined in this PR, the following tests were done on the full PR sequence once the entire namespace feature was implemented for typescript:
Feature Test Checklist
Module:
Client
CLI
Migration
Commit Log