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.

Sign up for GitHub

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

Jump to bottom

Move connection events to reducers #309

Merged
merged 9 commits into from
Oct 3, 2023
Merged

Move connection events to reducers #309

merged 9 commits into from
Oct 3, 2023

Conversation

RReverser
Copy link
Contributor

@RReverser RReverser commented Sep 20, 2023 •
edited
Loading

Description of Changes

(WIP) This changes __identity_connected__ and __identity_disconnected__ to become plain reducers instead of Wasm exports.

API and ABI

  • This is a breaking change to the module ABI
  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • This is a breaking change to the SDK API

If the API is breaking, please state below what will break

@RReverser RReverser marked this pull request as draft September 20, 2023 19:41
@RReverser RReverser force-pushed the connection-reducers branch 2 times, most recently from 25c8b98 to 77593a2 Compare September 27, 2023 15:41
@RReverser RReverser marked this pull request as ready for review September 27, 2023 15:50
@gefjon
Copy link
Contributor

gefjon commented Sep 27, 2023 •
edited
Loading

If you rename the bindings to be __client_connected__ and __client_disconnected__, then I won't have to do it in #299 , and we can avoid some merge conflicts.

@RReverser
Copy link
Contributor Author

RReverser commented Sep 27, 2023 •
edited
Loading

If you rename the bindings to be __client_connected__ and __client_disconnected__

Hm what do you mean? I don't see those names in #299, and those are no longer bindings - they're just reducers, so they get the optional context now like any other reducer.

@gefjon
Copy link
Contributor

gefjon commented Sep 27, 2023

I haven't made that change yet in #299, but I need to. Whether they're reducers or special functions, with #299 the name __identity_connected__ will no longer be accurate. (Arguably it was never accurate, since it was always possible to have multiple concurrent connections from the same identity, but now we're acknowledging that fact.)

@RReverser
Copy link
Contributor Author

RReverser commented Sep 27, 2023 •
edited
Loading

Ah ok... in that case let's just leave it for a separate PR since it's technically an improvement out of scope of either of those PRs. The smaller PRs, the easier to deal with said merge conflicts.

I don't mind rebasing mine if #299 lands first.

@gefjon
Copy link
Contributor

gefjon commented Sep 28, 2023

Ugh, that same mysterious should-be-unreachable failure in the SDK tests. Sigh.

cloutiertyler
cloutiertyler requested changes Sep 30, 2023
View reviewed changes
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the broken smoke tests.

@RReverser RReverser force-pushed the connection-reducers branch from 77593a2 to 85e0439 Compare October 2, 2023 12:58
@RReverser
Move connection events to reducers
f6ce4fa
@RReverser
More rebase fixups
a242c78
@RReverser
Avoid double-reference
b7ca93d
@RReverser
Filter out special reducers in generate cli
c6eac2f 
This updates filtering of `__init__` to exclude all special reducers, as well as moves the filtering to centralised place before calling language-specific generate command.
@RReverser RReverser force-pushed the connection-reducers branch from 6d489cb to c6eac2f Compare October 2, 2023 14:25
cloutiertyler
cloutiertyler approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cloutiertyler cloutiertyler enabled auto-merge (squash) October 3, 2023 22:08
@cloutiertyler cloutiertyler merged commit ba535f2 into master Oct 3, 2023
@RReverser RReverser deleted the connection-reducers branch October 3, 2023 23:22
RReverser added a commit to clockworklabs/spacetime-docs that referenced this pull request Oct 23, 2023
@RReverser
Update C# docs for connect/disconnect
5df59a1 
Update after the change in clockworklabs/SpacetimeDB#309.
@RReverser RReverser mentioned this pull request Oct 23, 2023
Merged
RReverser added a commit to clockworklabs/spacetime-docs that referenced this pull request Oct 25, 2023
@RReverser
Update C# docs for connect/disconnect (#9)
2968a4e 
Update after the change in clockworklabs/SpacetimeDB#309.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloutiertyler cloutiertyler cloutiertyler approved these changes

@gefjon gefjon Awaiting requested review from gefjon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RReverser @gefjon @cloutiertyler