feat!: cluster v1.0 support #40
Conversation
| * @param {BodyInit} [init.body] | ||
| * @param {AbortSignal} [init.signal] | ||
| */ | ||
| const streamRequest = async function* ( |
There was a problem hiding this comment.
If an error occurs during the stream, we set trailer headers. Is it possible to handle those with JS?
A good example is that cluster is restarted while you are querying something, and then perhaps the data stream is aborted and you get a trailer saying "context cancelled".
The trailer is X-Stream-Error. (at the beginning of the response there is a Trailer: X-Stream-Error header).
There was a problem hiding this comment.
AFAIK trailers have always been a problem in "browser" contexts. I'll double check.
There was a problem hiding this comment.
Still not supported using fetch mdn/browser-compat-data#14703
Not sure if we really want to go down the XMLHttpRequest route...
There was a problem hiding this comment.
It's not an overly important thing, for your usecase...
There was a problem hiding this comment.
Let's leave track on an issue and move on?
There was a problem hiding this comment.
Yes, talking to lidel he doesn't seem to think you can get them out of a XMLHttpRequest either.
(I read something that seemed to suggest you could)
There was a problem hiding this comment.
Note that soon our pattern of always set a X-Stream-Error trailer header and maybe send a X-Stream-Error trailer will be treated as an invalid response in nodejs in the next release (>=17.x) unless we get them to change their mind on it.
nodejs/undici#432 (comment)
web3-storage/web3.storage#1017
| buffer += decoder.decode(result.value, { stream: true }) | ||
| const parts = buffer.split(matcher) | ||
| buffer = parts.pop() || '' | ||
| for (const part of parts) yield JSON.parse(part) |
There was a problem hiding this comment.
we have some prior art in ipfs-utils around this https://github.com/ipfs/js-ipfs-utils/blob/master/src/http.js#L213-L235
There was a problem hiding this comment.
I based it on iterable-ndjson which looks almost identical. However that takes an AsyncIterable and we have a ReadableStream (which is not yet async iterable in the browser)
|
We could always set it too, but leave it empty.
Sent from ProtonMail mobile
…-------- Original Message --------
On Mar 30, 2022, 12:43, Oli Evans wrote:
@olizilla commented on this pull request.
---------------------------------------------------------------
In [src/index.js](#40 (comment)):
> @@ -298,6 +300,42 @@ const request = async (
}
}
+/**
+ * @param {API.Config} cluster
+ * @param {string} path
+ * @param {Object} init
+ * @param {string} [init.method]
+ * @param {Record<string, string|number|boolean|null|undefined>} [init.params]
+ * @param {BodyInit} [init.body]
+ * @param {AbortSignal} [init.signal]
+ */
+const streamRequest = async function* (
Note that soon our pattern of always set a X-Stream-Error trailer header and maybe send a X-Stream-Error trailer will be treated as an invalid response in nodejs in the next release (>=17.x) unless we get them to change their mind on it.
[nodejs/undici#432 (comment)](nodejs/undici#432 (comment))
—
Reply to this email directly, [view it on GitHub](#40 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAH2XTSQV6RF5MFRFC5SW3DVCQV5BANCNFSM5ROJYTCQ).
You are receiving this because you commented.Message ID: ***@***.***>
|
src/index.js
Outdated
| throw new Error('response data is not an array') | ||
| const statuses = [] | ||
| for await (const d of stream) { | ||
| statuses.push(toStausResponse(d)) |
There was a problem hiding this comment.
Not from this PR, but wat is Staus?
| statuses.push(toStausResponse(d)) | |
| statuses.push(toStatusResponse(d)) |
@hsanjuan that's a good idea. First I'm gonna see if we can get any movement on changing the http client in node. It's no landed in a LTS node yet. |
Support for Cluster v1.0.
There are breaking changes to some API endpoints (they now return ndjson).
BREAKING CHANGE: The client is not compatible with Cluster pre v1.0 anymore.