feat: support Node.js for @jupyterhub/binderhub-client#2044
feat: support Node.js for @jupyterhub/binderhub-client#2044agoose77 wants to merge 3 commits 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! |
|
So, I've fixed the tests ... but it is not perfect:
I can't seem to find an easy way to suppress these hooks or ask Jest to ignore the console messages. Perhaps one can patch the fetch error handling prototype or tweak the happy-dom console. |
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.