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

generate random wf id with math.random #25

Merged
merged 1 commit into from
Nov 14, 2024
Merged

generate random wf id with math.random #25

merged 1 commit into from
Nov 14, 2024

Conversation

CahidArda
Copy link
Collaborator

fixes #24

@CahidArda CahidArda merged commit 6dffd6d into main Nov 14, 2024
17 checks passed
@CahidArda CahidArda deleted the use-math-random branch November 14, 2024 14:42
@hckhanh
Copy link
Contributor

hckhanh commented Nov 14, 2024

hi @CahidArda , why don't you use nanoid directly instead of using the self-defined function?

@hckhanh
Copy link
Contributor

hckhanh commented Nov 14, 2024

For the crypto.getRandomValues
I see it's currently supported natively by cloudflare now.

https://developers.cloudflare.com/workers/runtime-apis/web-crypto/#:~:text=crypto.getRandomValues(bufferArrayBufferView)%20%3A%20ArrayBufferView

@CahidArda
Copy link
Collaborator Author

yes it was recently added to cloudflare, but we want to support older versions too.

when we use crypto directly, it doesn't work in Node 18. It gets crypto is not defined error.

Using nanoid had a similar issue if I remember correctly.

@hckhanh
Copy link
Contributor

hckhanh commented Nov 15, 2024

@CahidArda For the recent updates, I can use nanoid now without adding anything about node compatibility api.
So I think that you can use nanoid directly.
https://github.com/ai/nanoid/blob/main/index.browser.js

@CahidArda
Copy link
Collaborator Author

I will check again, thanks! @hckhanh

We released 0.1.5 with this update, we can change back to nanoid if it works well for us.

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.

Cannot bundle Node. js built-in "node: crypto"
3 participants