-
Notifications
You must be signed in to change notification settings - Fork 109
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
Improve TypeScript generated code (fixes and improvements) #603
Conversation
Thanks to that devs could now use it more intuitively, similarly to Rust, for example: let role = Role.Admin; // it returns { tag: "Admin", value: undefined }; let role1 = Role.Custom("Foo") // returns { tag: "Custom", value: "Foo" };
In JS all numbers are 64bit floats, but that means integers are only up to 53 bits. Thus any 64+ bits integers need to be handled as BigInts
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 like all these changes. I'd like to see a corresponding docs PR to update the SDK ref (and quickstart, if necessary) to reflect the new interface. I'd especially like the docs on Table.with
to be very clear about whether that mutates global state (I believe it doesn't), with an example that describes the behavior of:
MyTable.onInsert(someCallback)
MyTable.with(conn).onInsert(otherCallback)
MyTable.onInsert(thirdCallback)
I also notice that the corresponding TypeScript SDK PR doesn't update examples/quickstart
. The main way I'll be convinced that this is ready for the big time is when the quickstart client builds and runs.
Some of the methods were moved to the parent classes
@gefjon sorry for a late reply and thanks a lot for a review! I updated the quickstart guide in the SDK PR, but there are almost no changes to make it work. The |
So the only changes in the actual app code for quickstart is this diff, ie. adding |
Signed-off-by: Piotr Sarnacki <drogus@gmail.com>
Description of Changes
This is a set of changes improving various parts of TypeScript autogen:
User.with(client).onInsert(...)
SpacetimeDBClient.registerReducers
or.registerTables
. The problem with doing it automagically is that it will sometimes fail. Node will filter out any imports that are unused, so if you import a table without actually using the class constant, but you do a subscribe, it would result in an error. With explicit include it's no longer a problemfoo(user_type: UserType)
whereUserType
is an enum, in javascript you would have to do sth likeFooReducer.call({ tag: "Admin", value: undefined })
, which requires you to know how sum types are constructed in TypeScript. With the changes from the PR you can now doFooReducer.call(UserType.Admin)
. For an enum variant with value it would be sth likeAnEnum.VariantA("a value")
which will return{ tag: "VariantA", value: "a value" }
__identity_bytes
to camel case, which resulted in an errorargs: any[]
array we now list all of the values with proper types. So for example given anadd(u64, u64)
reducer the callback signature for a callback would be sth like(reducerEvent: ReducerEvent, a: BigInt, b: BigInt)
API and ABI breaking changes
These changes will break the TypeScript SDK, so a new release will be needed when this is released.
Expected complexity level and risk
I think it's a 2, it's not overly complex and it will actually lower some of the complexity in the SDK (for example by being more explicit with registering reducers and tables)