JS: Use async generator instead of callbacks + add tests #1773
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JS now also supports writing async code with [Asnc Generators](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AsyncGenerator) which work very much like how async code would in Python. This allows us to write code that flows better, is more familiar to python folks, and avoids messy callback hell. This code is a *straight* port of the existing callback based code to use an Async Generator instead. The EventIterator library is a very helpful convenience function here, allowing us to translate the callbacks from EventSource into an async generator without having to write a queue of promises ourselves. The iterator is stopped exactly the same way the callbacks were earlier stopped (whenever .stop() is called). All the callbacks have been switched over to a switch / case statement instead. We can get cleaner and use exceptions to handle failures, instead of the 'failure' case, but that can come in a later PR so this is a purely mechanical refactoring from one async method to another. An appropriate test for this is also added. To fully test the EventSource flow, we create a temporary web server that will serve a real captured eventsource response, with 1ms gaps between lines to fully simulate how this would work in reality. This gives us much more confidence about how this would work than mocking. With this, @jupyterhub/binder-client now has 100% unit test coverage! Ref jupyterhub#774
|
I added a screen recording of me testing this as well. |
yuvipanda
commented
Oct 10, 2023
consideRatio
added
maintenance
consideRatio
approved these changes
Oct 12, 2023
|
I'm not soo comfortable reviewing the JS code, but I think this makes sense and I didn't spot any issues are carefully going through this PR. Thank you @yuvipanda for working this! This seems great!! |
|
I think its a good call to opt for a merge at this point even though I'm not so confident reviewing this, it helps with Yuvi's contribution momentum and we aren't introducing new API or similar that we commit to long term in this PR either. |
consideRatio
pushed a commit
to jupyterhub/helm-chart
that referenced
this pull request
Oct 12, 2023
jupyterhub/binderhub#1773 Merge pull request #1773 from yuvipanda/async-gen
This was referenced Oct 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JS now also supports writing async code with Asnc Generators which work very much like how async code would in Python. This allows us to write code that flows better, is more familiar to python folks, and avoids messy callback hell.
This code is a straight port of the existing callback based code to use an Async Generator instead. The EventIterator library is a very helpful convenience function here, allowing us to translate the callbacks from EventSource into an async generator without having to write a queue of promises ourselves. The iterator is stopped exactly the same way the callbacks were earlier stopped (whenever .stop() is called).
All the callbacks have been switched over to a switch / case statement instead. We can get cleaner and use exceptions to handle failures, instead of the 'failure' case, but that can come in a later PR so this is a purely mechanical refactoring from one async method to another.
An appropriate test for this is also added. To fully test the EventSource flow, we create a temporary web server that will serve a real captured eventsource response, with 1ms gaps between lines to fully simulate how this would work in reality. This gives us much more confidence about how this would work than mocking.
With this, @jupyterhub/binder-client now has 100% unit test coverage!
Ref #774
Here is a video of me testing this. I tested both something that worked, and an image that is known to fail (RStudio on ARM).
Screen.Recording.2023-10-10.at.12.40.52.AM.mov