chore (ts-client): validate required Electric headers on response #1957
Conversation
There was a problem hiding this comment.
Looks good, I like the reuse of the fetch wrapper pattern!
I think possibly only 409 responses would also require an electric-handle to ensure it is possible to switch to the "right" shape, but I think doing just 200s is good enough to solve the issue of potentially misconfigured proxies
|
Can we also validate an Not having the cursor param can result in re-requesting to the same URL. |
|
Plus can we please write a section in the https://electric-sql.com/docs/guides/troubleshooting guide about this and include a link to it in the error message? Basically we would want to explain that if headers are missing then your proxy needs to make sure it includes them, maybe with a code sample showing how (Kyle has some proxy code that clones the headers). I can write this if you'd like. |
There was a problem hiding this comment.
What did y'all think also about the idea of also tracking which URLs we request and check that we don't request the same URL twice (within a given session)?
This would catch any other issue we haven't thought of.
The only tricky thing here I can think of is we'd need to wipe the Set on shape rotation and not do the check obviously if we're retrying on errors.
| missingHeaders.forEach((h) => { | ||
| msg += `- ${h}\n` | ||
| }) | ||
| msg += `\nThis is often due to a proxy not allowing all headers to be returned.` |
There was a problem hiding this comment.
| msg += `\nThis is often due to a proxy not allowing all headers to be returned.` | |
| msg += `\nThis is often due to a proxy not setting CORS correctly so that all Electric headers can be read by the client.` |
| export const requiredElectricResponseHeaders = [ | ||
| `electric-offset`, | ||
| `electric-handle`, | ||
| `electric-schema`, |
There was a problem hiding this comment.
electric-cursor & electric-up-to-date are also required.
There was a problem hiding this comment.
Our API spec says that the cursor is optional on the response, is that wrong?
There was a problem hiding this comment.
As above I believe we only return a cursor when returning a 204 to a live request.
There was a problem hiding this comment.
We actually include electric-cursor for all live requests, 200 or 204 - but only live requests
There was a problem hiding this comment.
Yeah ok it's a bit more tricky to check for these but it's still really bad if they're not available so it it's hard to check that'd be good. But with docs on what headers CORS allows through, probably just checking the minimal always sent set is enough
There was a problem hiding this comment.
The point is the dev would see an error while developing and fix things.
There was a problem hiding this comment.
So, the docs are correct and electric-cursor is optional as it is only provided in live mode. I could modify the check such that it checks if the original URL contains the live query param and only then check for the presence of electric-cursor.
@KyleAMathews Regarding electric-up-to-date, it is only present on the last chunk we will get from the sync service, so we don't want to check this on every message?
|
Checking just 200s for headers seems fine as it covers all the headers. |
Fixes #1950
Questions:
They are currently only being checked on 2XX responses.
Currently we check
electric-offset,electric-handle, andelectric-schema