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

Rust client SDK: rework EventContext into multiple types #2189

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Jan 29, 2025

Description of Changes

A repeated pain point with the post-0.12 client SDK API
has been excessive unnecessary pattern-matching or unwrapping
on the ctx.event within reducer callbacks.
Previously, these had ctx.event: Event,
with the type system not knowing that said Event would always be Event::Reducer.
This was in a misguided attempt to reduce proliferation of context types.

As of this PR, we have different event context types for each category of callback,
which allows us to inform the type system precisely of the type of event.

The event context types:

  • EventContext, passed to row callbacks.
    • Exactly the previous type.
    • In the future we may examine enum Event more closely and see if any of its variants are unreachable in row callbacks. If they are, we can deprecate those variants.
  • ReducerEventContext, passed to reducer callbacks.
    • event: ReducerEvent<Reducer>.
  • SubscriptionEventContext, passed to subscription applied and unapplied callbacks.
    • No event field needed, or event: ().
  • ErrorContext, passed to various on-error callbacks.
    • event: Error.

Changes to callbacks:

  • DbConnectionBuilder::on_disconnect now takes FnOnce(&ErrorContext).
    • For "normal" disconnects, this will have event: Error::Disconnected.
    • Is this better than having a separate DisconnectContext, which would likely also hold event: Error?
  • DbConnectionBuilder::on_connect_error now takes FnOnce(&ErrorContext).
  • on_{reducer} now takes FnMut(&ReducerEventContext, ...Args).
  • SubscriptionBuilder::on_applied and SubscriptionHandle::unsubscribe_then now take FnOnce(&SubscriptionEventContext).
  • SubscriptionBuilder::on_error now takes FnOnce(&ErrorContext).

Notes for reviewers

Based on #2169 , which in turn is based on #2111 . Use the "show changes from all commits" dropdown in the upper-left of the review window to isolate this PR's changes, which start from 8234155a.

Most of this diff, linewise, is in autogenerated code, which you can largely or completely ignore.

Question

I have kept all the event context types having a field ctx.event (except SubscriptionEventContext, which lacks any useful information to put there), of varying type. We could, alternatively, give this field a per-context name, like ctx.error for ErrorContext, ctx.reducer_event for ReducerEventContext, and... well, actually those are the only two that would change.

API and ABI breaking changes

Ayep, breaks the Rust client SDK API pretty comprehensibly. See crates/sdk/examples/quickstart-chat/main.rs for a sense of the changes.

Expected complexity level and risk

1: The changes are essentially mechanical and do not change any semantics. Our codegen is extensively tested.

Testing

  • I believe automated testing to be sufficient.

@gefjon gefjon added the api-break A PR that makes an API breaking change label Jan 29, 2025
@gefjon gefjon requested a review from Centril as a code owner January 29, 2025 16:33
@gefjon gefjon requested a review from jsdt January 29, 2025 16:33
@gefjon gefjon self-assigned this Jan 29, 2025
@gefjon gefjon force-pushed the phoebe/split-event-context branch from 8234155 to d041439 Compare February 3, 2025 16:02
@bfops bfops added the release-any To be landed in any release window label Feb 3, 2025
@kazimuth
Copy link
Contributor

kazimuth commented Feb 3, 2025

This is going to want to be rebased onto #2184 as well probably

@gefjon gefjon added release-1.0 and removed release-any To be landed in any release window labels Feb 4, 2025
A repeated pain point with the post-0.12 client SDK API
has been excessive unnecessary pattern-matching or unwrapping
on the `ctx.event` within reducer callbacks.
Previously, these had `ctx.event: Event`,
with the type system not knowing that said `Event` would always be `Event::Reducer`.
This was in a misguided attempt to reduce proliferation of context types.

As of this PR, we have different event context types for each category of callback,
which allows us to inform the type system precisely of the type of `event`.

The event context types:

- EventContext, passed to row callbacks.
  - Exactly the previous type.
  - In the future we may examine enum Event more closely and see if any of its variants are unreachable in row callbacks. If they are, we can deprecate those variants.
- ReducerEventContext, passed to reducer callbacks.
  - `event: ReducerEvent<Reducer>`.
- SubscriptionEventContext, passed to subscription applied and unapplied callbacks.
  - No event field needed, or `event: ()`.
- ErrorContext, passed to various on-error callbacks.
  - `event: Error`.

Changes to callbacks:

- `DbConnectionBuilder::on_disconnect` now takes `FnOnce(&ErrorContext)`.
  - For "normal" disconnects, this will have `event: Error::Disconnected`.
  - Is this better than having a separate `DisconnectContext`, which would likely also hold `event: Error`?
- `DbConnectionBuilder::on_connect_error` now takes `FnOnce(&ErrorContext)`.
- `on_{reducer}` now takes `FnMut(&ReducerEventContext, ...Args)`.
- `SubscriptionBuilder::on_applied` and `SubscriptionHandle::unsubscribe_then` now take `FnOnce(&SubscriptionEventContext)`.
- `SubscriptionBuilder::on_error` now takes `FnOnce(&ErrorContext)`.
@gefjon gefjon force-pushed the phoebe/split-event-context branch from 74964f9 to 22c99ae Compare February 5, 2025 00:28
@gefjon gefjon enabled auto-merge February 5, 2025 00:29
@gefjon gefjon added this pull request to the merge queue Feb 5, 2025
Merged via the queue into master with commit 2d90657 Feb 5, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break A PR that makes an API breaking change release-1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants