search

jupyterhub / binderhub

Run your code in the cloud, with technology so advanced, it feels like magic!
https://binderhub.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.57k stars 390 forks source link

JS: Use async generator instead of callbacks + add tests #1773

Closed yuvipanda closed 1 year ago

yuvipanda commented 1 year ago

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 https://github.com/jupyterhub/binderhub/issues/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).

https://github.com/jupyterhub/binderhub/assets/30430/2ce22011-ee53-4c27-b9b8-1ee27ba13c63

yuvipanda commented 1 year ago

I added a screen recording of me testing this as well.

consideRatio commented 1 year ago

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!!

consideRatio commented 1 year ago

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.