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

MathUtils: Use crypto.randomUUID() when available. #22556

Merged
merged 6 commits into from
Sep 20, 2021
Merged

MathUtils: Use crypto.randomUUID() when available. #22556

merged 6 commits into from
Sep 20, 2021

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Sep 20, 2021

Related issue: #13069

Description

Browser are adding a function for generating random UUIDs.
It's already available in Chrome, Edge and it's coming in Safari and Firefox.

@mrdoob mrdoob added this to the r133 milestone Sep 20, 2021
@mrdoob
Copy link
Owner Author

mrdoob commented Sep 20, 2021

Meh, I don't know how to make this pretty without breaking...

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 20, 2021

You probably have to put the test 'randomUUID' in crypto inside generateUUID().

@mrdoob
Copy link
Owner Author

mrdoob commented Sep 20, 2021

I was trying to avoid not having to check every time. I guess I'll have to declare it outside...

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 20, 2021

Maybe having two functions generateUUIDCrypto() and generateUUIDLegacy(). And then do this:

const generateUUID = ( 'randomUUID' in crypto ) ? generateUUIDCrypto : generateUUIDLegacy;

@mrdoob
Copy link
Owner Author

mrdoob commented Sep 20, 2021

I'm also trying to avoid having to declare _lut 😅

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 20, 2021

It seems the crypto variable does not exists in node.js. Adding typeof crypto=== 'undefined' should solve this.

@mrdoob
Copy link
Owner Author

mrdoob commented Sep 20, 2021

Alright, lets see how this goes 🤞

@mrdoob mrdoob merged commit 5aa01f2 into dev Sep 20, 2021
@mrdoob mrdoob deleted the mrdoob-patch-1 branch September 20, 2021 12:45
@mrdoob
Copy link
Owner Author

mrdoob commented Sep 20, 2021

Hmm, rollup is adding a getter:

Screen Shot 2021-09-20 at 7 18 35 PM

@Mugen87 I'll try your approach 👍

@mrdoob
Copy link
Owner Author

mrdoob commented Sep 20, 2021

e245401

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 20, 2021

Looking good!

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.

2 participants