-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: use uuid package instead of crypto for remote http jupyters #245
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use uuid only as a fallback? Or even just use a pseudo random counter? It really isn't necessary to have something complicated here as we just try to match requests.
I believe the uuid package already implements the function by falling back to the native crypto implementation if it is accessible. In particular, I looked into the generated
so notably the uuid package implementation gets bundled in (i.e. to my knowledge there is no perf benefit from explicitly handling the fallback.) I defer to you on whether you prefer to do something Math.random based, but the StackOverflow answer seemed to dissuade people from trying something like that (relevant part quoted below)
Let me know what you think and we can either merge as is or I can implement your suggestion. |
I rebased the fix on top of some of the latest changes - @domoritz if you get a sec, let me know if you have thoughts on my previous message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Looks good. I restarted the CI so hopefully that passes now (seemed to have been a random failure).
Closes #244