Skip to content
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

Implemented the new EventContext types in Typescript codegen #2224

Merged
merged 6 commits into from
Feb 8, 2025

Conversation

cloutiertyler
Copy link
Contributor

@cloutiertyler cloutiertyler commented Feb 7, 2025

Description of Changes

Relatively small PR. Corresponds to clockworklabs/spacetimedb-typescript-sdk#132

This PR generates a few new types based on new types added to the TypeScript SDK. Namely:

        "ReducerEventContextInterface",
        "SubscriptionEventContextInterface",
        "ErrorContextInterface",
        "SubscriptionBuilderImpl",

It also fixes a bug where SubscriptionBuilder was not actually type aware of the module types, so onApplied(ctx => ctx.reducers did not have the reducer types.

I have also improved the lint ignoring which in general reduces noise. I've wanted to do that for a while and since it helped debug here, I wanted to take the opportunity to merge it.

API and ABI breaking changes

This is a TypeScript API breaking change because now:

  • ctx.on<reducer> passes in a ReducerEventContext instead of an EventContext into its callback
  • subscriptionBuilder.onApplied passes in a SubscriptionEventContext into its callback
  • subscriptionBuilder.onError passes in an ErrorContext into its callback

Note that onConnectError is of the form:

 onConnectError(
    callback: (connection: DBConnection, error: Error) => void
  ): DBConnectionBuilder<DBConnection> {
}

@gefjon is that correct? I believe this is what I was told to do in the previous implementation but it looks like this is different in the Rust version:

.on_connect_error(|ctx| panic!("Error while connecting: {}", ctx.event))

This seems like quite a confusing API to be forced to look into the ErrorContext when the error could also be surfaced as an argument. Surely the following would be better (and more aligned with the ReducerArgs handling for the reducer callback):

 onConnectError(
    callback: (ctx: ErrorContext, error: Error) => void
  ): DBConnectionBuilder<DBConnection> {
}

In TypeScript the only meaningful difference beyond what I posted above is that the error is also available under the event field of the ErrorContext. Otherwise DBConnection == ErrorContext

Expected complexity level and risk

2

Testing

TODO pending Phoebe's reply

@gefjon
Copy link
Contributor

gefjon commented Feb 7, 2025

What's currently implemented in Rust is my intended design. I would not say I'm married to it, but I think it's fine, and at this point I consider the cost to change it quite high, as it would require PRs to all of public, private and docs.

My reasoning for the design I chose:

I think that having two distinct-but-equivalent ways to access the error will confuse users, and I don't see it as having a significant gain. Reading the error out of the context is easy. Keeping as much as possible inside the contexts is beneficial because we have more freedom to extend them in the future, whereas we essentially cannot change callback arglists post 1.0.

more aligned with the ReducerArgs handling for the reducer callback

Reducer args are different from the error in that we don't have a clean way to make them accessible inside the context. Even after this change, reading them from the context requires pattern-matching to read a known variant from a nested field, which is tedious and verbose. However, reducer callbacks get metadata in addition to the arguments, e.g. sender identity and connection ID, which is only passed inside the context, not provided as unboxed arguments.

@cloutiertyler
Copy link
Contributor Author

I think that having two distinct-but-equivalent ways to access the error will confuse users

Personally, I think having to access the error by ctx.event will confuse and surprise users. I think that if we surfaced the error as a second argument, people would quite rightly view the ErrorContext as the DBConnection primarily, and the error as the error.

I would strongly recommend surfacing the error as it's own argument in Rust.

@kazimuth
Copy link
Contributor

kazimuth commented Feb 7, 2025

I don't think that is a good idea. It's better to do the change in a forwards-compatible manner via the context. Users will need to look inside the contexts for other information anyway so it's good to encourage them to do so.

@cloutiertyler
Copy link
Contributor Author

cloutiertyler commented Feb 8, 2025

@kazimuth I've made the decision on this one and we decided to go with error as its own argument at least in the TypeScript API the description of: #2229

@cloutiertyler cloutiertyler marked this pull request as ready for review February 8, 2025 00:29
@cloutiertyler cloutiertyler added this pull request to the merge queue Feb 8, 2025
Merged via the queue into master with commit 249910b Feb 8, 2025
13 checks passed
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