-
-
Notifications
You must be signed in to change notification settings - Fork 600
fix: crypto.randomUUID causes React-Native build failures
#2858
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
base: alpha
Are you sure you want to change the base?
Changes from all commits
a79469c
1b8aa51
7ba5b9b
fe8a508
6e4fb95
0855c31
7a51152
2865cd0
9b16ba2
e14ed7f
734dca1
488fa4f
829b785
cdbe45b
4cac018
d42660a
b3c41db
effdd62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,25 +1,11 @@ | ||
| let uuid: () => string; | ||
|
|
||
| if (process.env.PARSE_BUILD === 'weapp') { | ||
| uuid = function () { | ||
| const s: string[] = []; | ||
| const hexDigits = '0123456789abcdef'; | ||
|
|
||
| for (let i = 0; i < 36; i++) { | ||
| s[i] = hexDigits.substr(Math.floor(Math.random() * 0x10), 1); | ||
| } | ||
|
|
||
| s[14] = '4'; // bits 12-15 of the time_hi_and_version field to 0010 | ||
| s[19] = hexDigits.substr((Number(s[19]) & 0x3) | 0x8, 1); // bits 6-7 of the clock_seq_hi_and_reserved to 01 | ||
| s[8] = s[13] = s[18] = s[23] = '-'; | ||
| return s.join(''); | ||
| }; | ||
| } else if (process.env.PARSE_BUILD === 'browser') { | ||
| // Use native crypto.randomUUID() from the Web Crypto API in browsers | ||
| uuid = () => globalThis.crypto.randomUUID(); | ||
| } else { | ||
| // Use Node.js crypto.randomUUID() for Node and React Native builds | ||
| uuid = require('crypto').randomUUID; | ||
| } | ||
| const uuid = (): string => { | ||
| if (typeof crypto === 'undefined' || typeof crypto?.randomUUID !== 'function') { | ||
| throw new Error( | ||
| 'crypto.randomUUID is not available in this environment. ' + | ||
| 'Use a UUID polyfill or environment-specific implementation (for example, in React Native you can import "react-native-random-uuid").' | ||
| ); | ||
| } | ||
| return crypto.randomUUID(); | ||
| }; | ||
|
Comment on lines
+1
to
+9
|
||
|
|
||
| export default uuid; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,2 @@ | ||
| declare let uuid: () => string; | ||
| declare const uuid: () => string; | ||
| export default uuid; |
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.
The test coverage is missing verification that the uuid module works correctly in React Native test environments. While the browser test verifies the error case when crypto is unavailable, there should also be a test in
react-native-test.js(similar to the weapp test atsrc/__tests__/weapp-test.js:58-62) to verify that uuid generation works properly when the crypto global is available or appropriately polyfilled in React Native environments. This would ensure the module behaves correctly across all supported environments.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.
@coderabbitai is it even easily possible to write a React Native test in a Node.js env?
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.
🧩 Analysis chain
🏁 Script executed:
Length of output: 2916
🏁 Script executed:
Length of output: 2682
@mtrezzaYes, it's definitely feasible to write React Native tests in a Node.js/Jest environment. The codebase already does this extensively insrc/__tests__/react-native-test.js, which setsprocess.env.PARSE_BUILD = 'react-native'and mocks React Native-specific modules (like the EventEmitter, CryptoController, StorageController, etc.).However, in this specific case, adding a React Native-specific UUID test would provide limited additional value because:
The new implementation is environment-agnostic: After this PR, the uuid module simply checks for
crypto.randomUUIDat runtime—there's no React Native-specific code path anymore.The browser tests already cover both scenarios:
crypto.randomUUIDis available (Line 158-163)crypto.randomUUIDis unavailable (the new test in this PR, Line 165-179)React Native would exhibit identical behavior: Whether running in React Native or browser, the module will either use the available
crypto.randomUUID(if polyfilled) or throw the same error (if not).If you wanted to add a React Native test for consistency with other environments (like weapp has on Line 58-62), it would just verify that uuid generation works when crypto is available—essentially duplicating what the browser test already covers. The weapp test is simpler because it only checks uniqueness, not the error case.
🧬 Code Graph Analysis Results
[src/Parse.ts] 291-297
[src/Parse.ts] 135-145
[integration/test/ParseEventuallyQueueTest.js] 4-4
[integration/test/ParseLegacyTest.js] 3-3
[integration/test/ParseReactNativeTest.js] 3-3
[integration/test/ParseReactNativeTest.js] 4-4
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.
So we won't add a test here, the error message plus breaking change entry should be enough.
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.