Skip to content

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.

5 participants