fix: always throw errors & always use named Error classes #1985
Conversation
KyleAMathews
changed the title
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
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. |
|
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. |
|
Also if we do observables we need the third parameter for the stream complete callback. |
KyleAMathews
force-pushed
the
throw-error
branch
from
28c4685 to
f76e167
Compare
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I need to update a few tests to catch errors that are thrown. |
|
Hmm actually these are all 404s — which suggests they're related to the multi-tenant work? |
| export class MissingShapeUrlError extends Error { | ||
| constructor() { | ||
| super(`Invalid shape options: missing required url parameter`) | ||
| this.name = `MissingShapeUrlError` |
There was a problem hiding this comment.
Should we also overwrite this.name in FetchBackoffAbortError?
There was a problem hiding this comment.
Oh right, yeah missed that one.
If they are multi tenant related they should be gone once we extract multi tenancy to a separate app. |
There was a problem hiding this comment.
This looks much clearer and more explicit and great to have the specific errors documented.
One observation is that this leads to two places where there is similar error handling: one is on instantiation and one is in the subscribe handler. I see the types of error handling may be different for the two locations -- I.e.: validate params on instantiate, handle FetchError ongoing. So I think this makes sense. But just to float the alternative, it could perhaps be possible to have a single try catch around a stream.run() function.
I know you're questioning the subscribe handler in the comments here. I do think it would be helpful to have a definitive pattern for how to handle 4xx errors from downstream Shape / useShape style functions.
In reality, there are circumstances where 4xx errors are transient or fixed on retry. For example, where they will be fixed by time passing or a new auth token, etc. When you have a component with a useShape binding in a local app, the sync binding needs to be absolutely bulletproof.
So I think we are going to have to expose a "handle FetchErrror when running" callback and I think we should have the result of that callback be able to determine whether the client continues with a backoff algorithm and also to potentially change the shape options. This could then be the mechanism to "restart" the shape stream, because actually if the return value is [true, {...updated_options...}] (or whatever) then the stream actually never stops.
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 |
|
Cool, let's get this merged. N.b.:
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 |
|
Ok I hadn't wrapped my head around this when I banged this out Friday evening — but yeah — calling So |
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 ) |
|
@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. |
|
Let's reconvene tomorrow to chat about this |
|
Not sure why all the react hook tests are failing 🤔 |
|
@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. |
Supersedes #1979 See: https://deploy-preview-1988--electric-next.netlify.app/docs https://deploy-preview-1988--electric-next.netlify.app/docs/guides/auth The Gatekeeper [client example code](https://deploy-preview-1988--electric-next.netlify.app/docs/guides/auth#example-1) will continue to work with #1985 but can be refactored if we change the control flow for handling stream-time fetch errors.
kevin-dp
force-pushed
the
throw-error
branch
from
a411441 to
a690940
Compare
|
I cherry picked 12fd091 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. |
|
Thanks for finishing this up @kevin-dp! I'm very much looking forward to always seeing nice clear errors now 😆 |
Fixes #1983