Skip to content
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

simplify for cloudflare workers #6

Closed
wants to merge 1 commit into from

Conversation

ryan-mars
Copy link

  • Fork from ulidx
  • Remove dependencies
  • Remove PRNG detection
  • Web Crypto API only

@ryan-mars ryan-mars closed this Nov 11, 2021
@ryan-mars ryan-mars deleted the cloudflare-workers branch November 11, 2021 02:43
@ryan-mars ryan-mars restored the cloudflare-workers branch November 11, 2021 02:52
@perry-mitchell
Copy link
Owner

@ryan-mars Why fork instead of offering an update to this library to work with cloudflare workers?

@ryan-mars
Copy link
Author

ryan-mars commented Dec 27, 2021

@perry-mitchell I don't recall what lead to the decision at the time but it wasn't meant as a slight. Would you like me to submit a PR?

@perry-mitchell
Copy link
Owner

perry-mitchell commented Dec 27, 2021 via email

@ryan-mars
Copy link
Author

I'm happy to contribute back. I apologize for creating yet-another-package. It was done out of expedience. I'll look through the code and submit a PR.

The two major differences between Cloudflare Workers and other environments are date seed behavior, and crypto.

Date.now

From the Workers' docs:

In Workers, you can get the current time using the JavaScript Date API, for example by calling Date.now(). However, the time value returned by this is not really the current time. Instead, it is the time at which the network message was received which caused the application to begin executing. While the application executes, time is locked in place.

crypto

Workers provides the Web Crypto API via crypto global. Workers does not run on node. Instead the runtime more resembles a web worker. Thus, using the window shortcut to get the current global scope (instead of self) within a Worker will return an error.

@ryan-mars ryan-mars reopened this Dec 30, 2021
@perry-mitchell
Copy link
Owner

Thanks @ryan-mars for the submitted work - I very much appreciate the gesture.

I can't of course merge this, as it drastically changes the library, removing a lot of the functionality and capability required for other platforms (node, web, web workers etc.). It does give me insight into how you've made it work in Cloudflare workers.

Workers provides the Web Crypto API via crypto global

This is interesting and something I need to look into. I have issues currently with this due to the Node REPL exposing the same property, and confusing the whole PRNG detection.

While the application executes, time is locked in place.

This sounds.. absolutely terrible. I don't understand why they'd choose to do this. All the same, I don't know how this library is meant to work when time can't be relied upon. It's a crucial part of the algorithm.

@perry-mitchell
Copy link
Owner

Closing this as I've addressed it in the relevant issue - #5.

@grempe
Copy link
Contributor

grempe commented Jan 1, 2022

I just wanted to say thanks to @ryan-mars for creating the fork that works with Workers. I came here to say it since issues/discussions seem to be closed on the fork repo. It's a useful tool for the large community of Workers developers.

🥇

Cheers.

@ryan-mars
Copy link
Author

@grempe Thank you so much for the kind feedback. I just want to make something useful to others. I'll make sure issues and discussion are open.

@grempe grempe deleted the cloudflare-workers branch February 13, 2022 18:57
@grempe grempe restored the cloudflare-workers branch February 13, 2022 18:57
@ryan-mars ryan-mars deleted the cloudflare-workers branch December 8, 2022 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants