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

chore: Refactor client #1711

Merged
merged 15 commits into from
Sep 17, 2024
Merged

chore: Refactor client #1711

merged 15 commits into from
Sep 17, 2024

Conversation

msfstef
Copy link
Contributor

@msfstef msfstef commented Sep 16, 2024

Extracted out refactoring from #1630

Had to also add a subscribeSync method on ShapeStream otherwise we cannot guarantee that Shape is up to date when the stream is up to date, since the processing is done asynchronously. We have three options:
1. We re-introduce the PromiseOr abstractions in order for the single subscribe call to handle both sync and async
2. We introduce an explicit subcsribeSync like I have done here, which provides strong guarantees on up-to-date synchronicity
3. We change Shape to maintain it's own "up to date" logic as it is not guaranteed that when the stream is up to date that the Shape will have materialized its local state fully, since processing is async

Reintroduced thin version of sync + async handling in the queue to maintain the current API and intended behaviour.

I've discussed with @kevin-dp and we think that actually making the processing callbacks create backpressure on the stream rather than the stream collecting the responses non-stop might be a better API and allow the developers to choose the pace at which the stream will collect data through the choice of processing callback. Will open a new PR for this, so we can merge this one with the refactor.

Copy link

netlify bot commented Sep 16, 2024

Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit 77b1d1a
🔍 Latest deploy log https://app.netlify.com/sites/electric-next/deploys/66e852e0ab1eec00085b0344
😎 Deploy Preview https://deploy-preview-1711--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@kevin-dp kevin-dp left a comment

Choose a reason for hiding this comment

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

Good work!
I left some comments but nothing major.
Perhaps, let's seek a third opinion (@KyleAMathews ) about the subscribeSync method vs not having that method and awaiting the callbacks instead (see my comment about that on L338 in client.ts).

packages/typescript-client/src/client.ts Show resolved Hide resolved
packages/typescript-client/src/client.ts Show resolved Hide resolved
packages/typescript-client/src/client.ts Show resolved Hide resolved
packages/typescript-client/src/client.ts Outdated Show resolved Hide resolved
packages/typescript-client/src/constants.ts Outdated Show resolved Hide resolved
packages/typescript-client/src/constants.ts Outdated Show resolved Hide resolved
packages/typescript-client/src/fetch.ts Show resolved Hide resolved
packages/typescript-client/src/offset.ts Outdated Show resolved Hide resolved
packages/typescript-client/test/offset.test.ts Outdated Show resolved Hide resolved
.changeset/dull-hornets-talk.md Outdated Show resolved Hide resolved
@thruflo
Copy link
Contributor

thruflo commented Sep 17, 2024

One observation is that the Typescript client is a canonical implementation for other clients to reference. Looking at the source in client.ts, I didn't see any commenting of this subscribe() vs subscribeSync() distinction.

If we have these two methods, then, both in the source and the website docs (api -> clients -> typescript) it would be good to be clear about why there are two methods, what they do, when to use one over the other and whether this is a common pattern for other clients to follow.

@msfstef
Copy link
Contributor Author

msfstef commented Sep 17, 2024

@thruflo the subscribeSync was a tentative solution, I've rolled it back in this PR and the API remains exactly the same but handles sync/async more correctly - I'll open a new PR with a suggested behaviour change that we've discussed with @kevin-dp that would still not change the API itself but would give meaning to the type of callback you pass to subscribe; i.e. async callbacks would actually be awaited by the stream and provide backpressure, rather than what we do now where all the stream is collected in memory regardless of the speed of processing of the subscribers

@thruflo
Copy link
Contributor

thruflo commented Sep 17, 2024

Cool 👍

I guess the "let's make sure we document clearly" point still applies to the alternative solution.

@msfstef msfstef merged commit 9992a74 into main Sep 17, 2024
23 checks passed
@msfstef msfstef deleted the msfstef/refactor-client branch September 17, 2024 09:53
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