[expo-random] Add sync equivalents for expo-random methods#9750
[expo-random] Add sync equivalents for expo-random methods#9750brentvatne merged 16 commits intomasterfrom
Conversation
|
Native Component List for this branch is ready |
brentvatne
force-pushed
the
@brent/expo-random-sync-methods
branch
from
30bd095 to
a2faeb0
Compare
|
|
||
| @ExpoMethod | ||
| public void getRandomBase64StringAsync(int randomByteCount, final Promise promise) { | ||
| private String _getRandomBase64String(int randomByteCount) throws NoSuchAlgorithmException { |
There was a problem hiding this comment.
Leave off the underscore for private Java methods
| if (result == errSecSuccess) { | ||
| return [bytes base64EncodedStringWithOptions:0]; | ||
| } else { | ||
| @throw([NSException exceptionWithName:@"expo-random" reason:@"Failed to create secure random string" userInfo:nil]); |
There was a problem hiding this comment.
Instead of throwing, accept an optimal NSError double pointer and set its value.
| export function getRandomBytes(byteCount: number): Uint8Array { | ||
| assertByteCount(byteCount, 'getRandomBytes'); | ||
| const validByteCount = Math.floor(byteCount); | ||
| if (ExpoRandom.getRandomBytesAsync) { |
sjchmiela
left a comment
sjchmiela
left a comment
There was a problem hiding this comment.
Good job! Happy to see integration with newest uuid! 👍
| include(":expo-random") | ||
| project(":expo-random").projectDir = new File("../packages/expo-random/android") No newline at end of file |
There was a problem hiding this comment.
Would you please move it to nearer include ':unimodules-test-core' etc.? This should be wrapped with // WHEN_DISTRIBUTING_REMOVE_{FROM,TO}_HERE, otherwise it'll end up in Android shell app (Turtle shouldn't try to use this project, instead, expokit prebuilt AARs are available there).
Co-authored-by: Stanisław Chmiela <[email protected]>
Co-authored-by: Stanisław Chmiela <[email protected]>
brentvatne
force-pushed
the
@brent/expo-random-sync-methods
branch
from
079d890 to
3f0b065
Compare
Co-authored-by: Stanisław Chmiela <[email protected]>
| const array = new Uint8Array(length); | ||
| return window.crypto.getRandomValues(array); | ||
| // @ts-ignore | ||
| return (window.crypto ?? window.msCrypto).getRandomValues(array) |
There was a problem hiding this comment.
| return (window.crypto ?? window.msCrypto).getRandomValues(array) | |
| return (window.crypto ?? window.msCrypto).getRandomValues(array); |
brentvatne
changed the title
|
👋 Hi! Just wanted to let you know that there is a conversation in Related to the performance issue, I was wondering if it wouldn't be possible to change from a string to an array of numbers. This way the |
|
How much does it matter or is this mostly a developer-driven desire rather than a UX-driven one? Either way in the future we should be able to build something faster than strings and numbers, we're just looking for something reasonable that can make UUIDs which are tiny regardless. |
|
The get-random-values is used in crypto libraries like sodium-javascript which tend to process quite a bit of data which can get really slow (stopped, stuttering UI) |
|
@martinheidegger - i'm open to a pr to improve perf here, i don't have time to work on it myself though. the main objective of this pr was to unblock people who need crypto.getRandomValues polyfill for usage with uuid and so on |
4 participants
Why
Provide building blocks to fix #7209
How
Added a sync version of the expo-random
getRandomBytesmethod.To do this I had to convert expo-random away from a unimodule, because we don't support sync methods yet. This was a bit awkward to do on the build/dependency configuration side because we lean on autolinking unimodules in our packages directory, so I'd appreciate feedback on whether I've configured that properly in the appropriate gradle files.
Test Plan
Open the "Random" example in NCL