search

electric-sql / electric

Sync little subsets of your Postgres data into local apps and services.
https://electric-sql.com
Apache License 2.0
6.48k stars 156 forks source link

fix: always throw errors & always use named Error classes #1985

Closed KyleAMathews closed 4 days ago

KyleAMathews commented 1 week ago

Fixes https://github.com/electric-sql/electric/issues/1983

netlify[bot] commented 1 week ago

Deploy Preview for electric-next ready!

Name Link
Latest commit 983815c3ce3bf9f129c565abeb169f70b43dce36
Latest deploy log https://app.netlify.com/sites/electric-next/deploys/6737ee51bb0612000826b9b9
Deploy Preview https://deploy-preview-1985--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.

KyleAMathews commented 1 week ago

I'm skeptical now too of our passing errors to the subscribe functions. We have no way of recovering from the errors and also try/catch is the normal way of seeing errors. I'll remove those in another pr unless people disagree.

KyleAMathews commented 1 week ago

It'd be nice to have an observable style interface as well but we're not using that anywhere so in practice right now we just lose the errors. Observables are the exception so people should opt into that interface.

KyleAMathews commented 1 week ago

Also if we do observables we need the third parameter for the stream complete callback.

netlify[bot] commented 1 week ago

Deploy Preview for electric-next ready!

Name Link
Latest commit 728b3b2f3aee5b240ef308c6a38b3e16e76d3e88
Latest deploy log https://app.netlify.com/sites/electric-next/deploys/673cd73baaf15b000841b160
Deploy Preview https://deploy-preview-1985--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.

KyleAMathews commented 1 week ago

I need to update a few tests to catch errors that are thrown.

KyleAMathews commented 1 week ago

Hmm actually these are all 404s — which suggests they're related to the multi-tenant work?

kevin-dp commented 1 week ago

Hmm actually these are all 404s — which suggests they're related to the multi-tenant work?

If they are multi tenant related they should be gone once we extract multi tenancy to a separate app. Planning to finish that today, once it's in main we could rebase to see if the errors persist.

KyleAMathews commented 1 week ago

But just to float the alternative, it could perhaps be possible to have a single try catch around a stream.run() function.

That's the Elixir client :-P — for JS, when you start a new ShapeStream, it immediately starts running.

I hear you about needing to make transient 401 errors easier to handle — you can handle it with a try/catch which then restarts the stream — the problem with this of course is then you have to restart the fetching every time and it's not particularly ergonomic.

Exposing a way to customize error handling makes sense too — it'd be easy to mess up the shape stream that way but the server would protect against that by forcing the client to refetch.

These are separate problems from what this PR is trying to fix so let's continue the discussion in #1982

thruflo commented 1 week ago

Cool, let's get this merged.

N.b.:

when you start a new ShapeStream, it immediately starts running.

When I read this I immediately thought: if you don't immediately pass the stream to a shape, you may miss messages from the stream that are fired before you register subscribers. Presumably this is an "add it when people ask for it" feature but an autoRun: false option + stream.run() combo may be useful at some point.

KyleAMathews commented 1 week ago

Ok I hadn't wrapped my head around this when I banged this out Friday evening — but yeah — calling run() from the constructor does mean that a try/catch around the new ShapeStream call won't catch any errors thrown while the stream is running...

So autoRun false would work or... just disable autoRun and make .run() an explicit step as then error catching is neatly split between constructing the class and running the class.

balegas commented 1 week ago

So autoRun false would work or... just disable autoRun and make .run() an explicit step as then error catching is neatly split between constructing the class and running the class.

I'm a strong advocate for separating creating and running the stream. There are errors you want to catch early because those are fatal errors and it's worthless to start the stream. In general any side effects from a constructor are hard to work with because you cannot await.

(cc @msfstef )

KyleAMathews commented 1 week ago

@balegas yeah I added an autoRun option but we could just make the two step process the default. This is a breaking change but now's the time to do that.

KyleAMathews commented 1 week ago

Let's reconvene tomorrow to chat about this

KyleAMathews commented 1 week ago

Not sure why all the react hook tests are failing 🤔

KyleAMathews commented 6 days ago

@kevin-dp I incorporated your changes but commented out the retry logic as that'll need tests & docs and this PR is getting way too long in the tooth as it is — so you or someone else can pick that up in a subsequent PR.

I'm out of time to work on this today — if someone wants to figure out why the react-hooks tests are all not working, that'd be great.

kevin-dp commented 4 days ago

I cherry picked 12fd091c4fde2972be0d691953dc53586bfc6d5a because it was conflicting with that PR which is in main and rebasing is a pain because of the enormous amounts of commits in the branch + some of those commits introduce the explicit start feature and then removes it in a commit later down the line which causes a lot of conflicts.

KyleAMathews commented 4 days ago

Thanks for finishing this up @kevin-dp! I'm very much looking forward to always seeing nice clear errors now 😆