feat: support Node.js for @jupyterhub/binderhub-client#2044
feat: support Node.js for @jupyterhub/binderhub-client#2044agoose77 wants to merge 1 commit intojupyterhub:mainfrom
@jupyterhub/binderhub-client#2044Conversation
|
|
||
| this.eventIteratorQueue = null; | ||
| this.abortSignal = null; | ||
| this.stopQueue = null; |
There was a problem hiding this comment.
The intention here is that the queue is the application-level interface we care about. The underlying EventSource is just an implementation detail.
| const response = await fetch(input, { | ||
| ...init, | ||
| headers: { ...init.headers, ...headers }, | ||
| }); |
There was a problem hiding this comment.
Ensure we pass through the headers
| // Known failures are passed on and handled in onError | ||
| if (response.ok) { | ||
| return response; | ||
| } else if ( | ||
| response.status >= 400 && | ||
| response.status < 500 && | ||
| response.status !== 429 | ||
| ) { | ||
| return response; | ||
| } | ||
| // Otherwise, throw, triggering a retry | ||
| throw new EventStreamRetry(); |
There was a problem hiding this comment.
OK responses, or known-failures, are passed through to EventSource, which dispatches the proper events.
We don't do that for retry-able responses, because EventSource doesn't feature a configurable retry mechanism — it only kicks in for failures during fetching.
| throw new EventStreamRetry(); | ||
| } | ||
| }, | ||
| this.stopQueue = () => queue.stop(); |
There was a problem hiding this comment.
Leak the stop action.
| return () => { | ||
| es.removeEventListener("message", onMessage); | ||
| es.removeEventListener("error", onError); | ||
| es.close(); |
There was a problem hiding this comment.
This closes the event source, from stopQueue().
|
@ agoose77 thanks for the pull request. Switches out @microsoft/fetch-event-source in favour of eventsource looks great. Two tests failed. Can you investigate the tests that failed? |
|
@rgaiacs yes, at this stage just pushing the code. Plan to get to the tests later today! |
2 participants
This PR switches out
@microsoft/fetch-event-sourcein favour ofeventsource. This package is both newer (and maintained) and explicitly supports the Node.js runtime.I'm consuming a vendored version of these changes in https://github.com/2i2c-org/clinder/tree/main/packages/binderhub-client-next, which is Node.js only.