-
-
Notifications
You must be signed in to change notification settings - Fork 353
Bump core to RN 0.80.1
#5181
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: main
Are you sure you want to change the base?
Bump core to RN 0.80.1
#5181
Conversation
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 20d5eaa | 377.62 ms | 406.50 ms | 28.88 ms |
| c08359e | 421.87 ms | 445.37 ms | 23.50 ms |
| 8490686+dirty | 365.59 ms | 400.24 ms | 34.65 ms |
| c1573b3 | 400.85 ms | 411.82 ms | 10.97 ms |
| c4e097a | 338.06 ms | 439.36 ms | 101.30 ms |
| 07808fb+dirty | 419.10 ms | 419.08 ms | -0.02 ms |
| 49ef936+dirty | 405.96 ms | 417.22 ms | 11.26 ms |
| a31630c+dirty | 408.91 ms | 416.80 ms | 7.89 ms |
| 459a438+dirty | 417.09 ms | 406.52 ms | -10.57 ms |
| 05bef0e+dirty | 349.78 ms | 334.04 ms | -15.74 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 20d5eaa | 17.75 MiB | 20.15 MiB | 2.41 MiB |
| c08359e | 17.75 MiB | 20.15 MiB | 2.41 MiB |
| 8490686+dirty | 17.75 MiB | 19.70 MiB | 1.96 MiB |
| c1573b3 | 17.75 MiB | 20.15 MiB | 2.41 MiB |
| c4e097a | 17.75 MiB | 19.68 MiB | 1.94 MiB |
| 07808fb+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| 49ef936+dirty | 17.75 MiB | 19.69 MiB | 1.94 MiB |
| a31630c+dirty | 17.75 MiB | 19.68 MiB | 1.94 MiB |
| 459a438+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| 05bef0e+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 90afdd3+dirty | 1233.90 ms | 1240.90 ms | 7.00 ms |
| 21c9e75+dirty | 1237.78 ms | 1247.66 ms | 9.88 ms |
| 6fee48d+dirty | 1222.14 ms | 1231.44 ms | 9.30 ms |
| 170d5ea+dirty | 1219.27 ms | 1231.90 ms | 12.63 ms |
| 1853710+dirty | 1224.35 ms | 1230.18 ms | 5.84 ms |
| bc9680d+dirty | 1221.41 ms | 1241.47 ms | 20.06 ms |
| 20d5eaa+dirty | 1231.12 ms | 1226.00 ms | -5.12 ms |
| 276d348+dirty | 1224.22 ms | 1227.38 ms | 3.16 ms |
| c7f264b+dirty | 1211.82 ms | 1218.04 ms | 6.22 ms |
| a31630c+dirty | 1229.09 ms | 1230.94 ms | 1.85 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 90afdd3+dirty | 2.63 MiB | 3.99 MiB | 1.35 MiB |
| 21c9e75+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 6fee48d+dirty | 2.63 MiB | 3.96 MiB | 1.33 MiB |
| 170d5ea+dirty | 2.63 MiB | 3.98 MiB | 1.35 MiB |
| 1853710+dirty | 2.63 MiB | 3.91 MiB | 1.28 MiB |
| bc9680d+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 20d5eaa+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 276d348+dirty | 2.63 MiB | 3.98 MiB | 1.34 MiB |
| c7f264b+dirty | 2.63 MiB | 3.91 MiB | 1.28 MiB |
| a31630c+dirty | 2.63 MiB | 3.98 MiB | 1.34 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8490686+dirty | 344.38 ms | 364.37 ms | 19.98 ms |
| 20daa0a+dirty | 352.33 ms | 424.30 ms | 71.98 ms |
| 818a608+dirty | 350.29 ms | 397.38 ms | 47.09 ms |
| 69602ce+dirty | 375.37 ms | 405.28 ms | 29.91 ms |
| b3b5b0d+dirty | 361.42 ms | 403.90 ms | 42.48 ms |
| 07808fb+dirty | 392.47 ms | 451.94 ms | 59.47 ms |
| 49ef936+dirty | 333.72 ms | 387.51 ms | 53.79 ms |
| a31630c+dirty | 359.89 ms | 416.90 ms | 57.00 ms |
| 459a438+dirty | 359.50 ms | 390.53 ms | 31.03 ms |
| 05bef0e+dirty | 451.63 ms | 533.90 ms | 82.27 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8490686+dirty | 7.15 MiB | 8.43 MiB | 1.28 MiB |
| 20daa0a+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| 818a608+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 69602ce+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| b3b5b0d+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 07808fb+dirty | 7.15 MiB | 8.43 MiB | 1.28 MiB |
| 49ef936+dirty | 7.15 MiB | 8.42 MiB | 1.26 MiB |
| a31630c+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 459a438+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| 05bef0e+dirty | 7.15 MiB | 8.43 MiB | 1.28 MiB |
| ], | ||
| "peerDependencies": { | ||
| "expo": ">=49.0.0", | ||
| "react": ">=17.0.0", |
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.
q: Why do we remove the "react" peer depencency?
| includeUpdates: true, | ||
| }), | ||
| expect.anything(), | ||
| expect.toBeNil() |
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.
q: Do we know why the second parameter changed? Does it affect the profiler?
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.
Thank you for you work on this @alwx 🙇
Overall the changes LGTM 🚀
I think the only blocking thing is the Native test failure on iOS. I haven't looked at it in detail but some of the errors might be related to the folly. We added some check here some time ago. Probably the Cocoa Tester needs something similar 🤔
33bf13b to
dd7d431
Compare
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| bc9680d+dirty | 1228.57 ms | 1233.64 ms | 5.07 ms |
| c9e95bd+dirty | 1205.83 ms | 1207.38 ms | 1.55 ms |
| c1573b3+dirty | 1245.69 ms | 1245.48 ms | -0.21 ms |
| 49ef936+dirty | 1221.27 ms | 1221.60 ms | 0.34 ms |
| 8d89cc9+dirty | 1232.35 ms | 1228.53 ms | -3.82 ms |
| e2fa43d+dirty | 1224.43 ms | 1235.40 ms | 10.98 ms |
| 9f211e3+dirty | 1215.38 ms | 1218.15 ms | 2.77 ms |
| 7be1f99+dirty | 1222.43 ms | 1217.15 ms | -5.28 ms |
| 21c9e75+dirty | 1206.20 ms | 1223.54 ms | 17.35 ms |
| 5c16cdc+dirty | 1235.67 ms | 1241.18 ms | 5.51 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| bc9680d+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| c9e95bd+dirty | 3.19 MiB | 4.44 MiB | 1.25 MiB |
| c1573b3+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 49ef936+dirty | 3.19 MiB | 4.54 MiB | 1.36 MiB |
| 8d89cc9+dirty | 3.19 MiB | 4.53 MiB | 1.35 MiB |
| e2fa43d+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 9f211e3+dirty | 3.19 MiB | 4.48 MiB | 1.29 MiB |
| 7be1f99+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 21c9e75+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 5c16cdc+dirty | 3.19 MiB | 4.53 MiB | 1.34 MiB |
f293655 to
5f9c0aa
Compare
|
@sentry review |
| }; | ||
| const mockedAppState: AppState & MockAppState = { | ||
| removeSubscription: jest.fn(), | ||
| listener: jest.fn(), | ||
| isAvailable: true, | ||
| currentState: 'active', | ||
| addEventListener: (_, listener) => { | ||
| mockedAppState.listener = listener; | ||
| return { | ||
| remove: mockedAppState.removeSubscription, | ||
| }; | ||
| }, | ||
| setState: (state: AppStateStatus) => { | ||
| mockedAppState.currentState = state; | ||
| mockedAppState.listener(state); | ||
| }, | ||
| }; | ||
| jest.mock('react-native/Libraries/AppState/AppState', () => mockedAppState); | ||
| jest.mock('react-native', () => { | ||
| const mockedAppState: AppState & MockAppState = { | ||
| removeSubscription: jest.fn(), | ||
| listener: jest.fn(), | ||
| isAvailable: true, | ||
| currentState: 'active', | ||
| addEventListener: jest.fn(), | ||
| setState: (state: AppStateStatus) => { | ||
| mockedAppState.currentState = state; | ||
| mockedAppState.listener(state); | ||
| }, | ||
| }; | ||
| return { | ||
| AppState: mockedAppState, | ||
| Platform: { OS: 'ios' }, | ||
| NativeModules: { | ||
| RNSentry: {}, | ||
| }, | ||
| }; |
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 mock setup creates a circular reference where mockedAppState.addEventListener and mockedAppState.setState reference mockedAppState before it's fully initialized. While this works in JavaScript due to hoisting, it's a code smell. Consider initializing addEventListener and setState after the object is created, or restructuring the mock to avoid this pattern.
Did we get this right? 👍 / 👎 to inform future reviews.
| jest.mock('react-native', () => { | ||
| const mockedAppState: AppState & MockAppState = { | ||
| removeSubscription: jest.fn(), | ||
| listener: jest.fn(), | ||
| isAvailable: true, | ||
| currentState: 'active', | ||
| addEventListener: jest.fn(), | ||
| setState: (state: AppStateStatus) => { | ||
| mockedAppState.currentState = state; | ||
| mockedAppState.listener(state); | ||
| }, | ||
| }; | ||
| return { | ||
| AppState: mockedAppState, | ||
| Platform: { OS: 'ios' }, | ||
| NativeModules: { | ||
| RNSentry: {}, | ||
| }, | ||
| }; | ||
| }); |
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.
Similar to the idleNavigationSpan test, the mock setup creates a circular reference in the object initialization. Consider restructuring the mock to avoid this pattern for better code clarity and maintainability.
Did we get this right? 👍 / 👎 to inform future reviews.
| showFeedbackButton(); | ||
|
|
||
| fireEvent.press(getByText('Report a Bug')); | ||
| await waitFor(() => fireEvent.press(getByText('Report a Bug'))); |
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.
On line 88, the pattern await waitFor(() => fireEvent.press(...)) is unusual. Typically, fireEvent.press() is synchronous and doesn't need to be wrapped in waitFor().
The waitFor() is usually used for waiting for asynchronous side effects after the event. Consider whether this should be fireEvent.press(...) followed by await waitFor(() => expect(...)) for better clarity.
Did we get this right? 👍 / 👎 to inform future reviews.
| await waitFor(() => { | ||
| fireEvent.press(getByText('Report a Bug')); | ||
| fireEvent.press(getByText('Take a screenshot')); | ||
| }); |
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.
Multiple instances of wrapping synchronous fireEvent.press() calls inside waitFor() (lines 109-112, 135-138, 167-170, 213-216) suggest a misunderstanding of the testing pattern. fireEvent operations are synchronous; only the assertions of their effects typically need waitFor(). This could lead to unnecessary delays in test execution.
Did we get this right? 👍 / 👎 to inform future reviews.
| includeUpdates: true, | ||
| }), | ||
| expect.anything(), | ||
| expect.toBeNil() |
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 change from expect.anything() to expect.toBeNil() appears to be checking for a nil/null value. However, toBeNil() is not a standard Jest matcher. This might be a custom matcher from jest-extended, but it should be verified that this change correctly validates the expected behavior. Standard Jest would use expect(value).toBeNull() or expect(value).toBeUndefined().
Did we get this right? 👍 / 👎 to inform future reviews.
| updateProps: {}, | ||
| }), | ||
| expect.anything(), | ||
| expect.toBeNil(), |
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.
Similar to the previous comment, verify that expect.toBeNil() is the correct matcher and is properly imported/configured. If this is from jest-extended, ensure it's properly set up in the test configuration.
Did we get this right? 👍 / 👎 to inform future reviews.
dev-packages/e2e-tests/cli.mjs
Outdated
| const versionNumber = parseFloat(rnVersion.replace(/[^\d.]/g, '')); | ||
| const shouldRunCodegen = platform === 'android' && versionNumber >= 0.80; | ||
| const shouldRunCodegen = platform === 'android' && versionNumber >= 0.69; |
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 version threshold change from 0.80 to 0.69 for running codegen needs to be carefully verified. According to the repository context, Metro version pinning is applied only for React Native versions 0.80 and above. This change to 0.69 might require corresponding updates to the Metro version pinning logic in rn.patch.package.json.js to ensure consistency. Verify that RN 0.69 doesn't have the same Metro issues mentioned in the context.
Did we get this right? 👍 / 👎 to inform future reviews.
dev-packages/e2e-tests/cli.mjs
Outdated
| const versionNumber = parseFloat(RNVersion.replace(/[^\d.]/g, '')); | ||
| if (versionNumber >= 0.80) { | ||
| if (versionNumber >= 0.69) { | ||
| execSync(`${patchScriptsDir}/rn.patch.package.json.js --path package.json`, { | ||
| stdio: 'inherit', | ||
| cwd: appDir, | ||
| env: env, | ||
| }); | ||
| } else { | ||
| console.log(`Skipping rn.patch.package.json.js for RN ${RNVersion} (< 0.80)`); | ||
| console.log(`Skipping rn.patch.package.json.js for RN ${RNVersion} (< 0.69)`); |
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 version threshold for applying rn.patch.package.json.js has been lowered from 0.80 to 0.69. According to the repository-specific context, Metro version pinning via this patch script is critical for RN 0.80+ to prevent E2E test failures. Ensure that applying this patch to RN 0.69 is safe and doesn't introduce unintended side effects, or update the patch script logic to handle different RN versions appropriately.
Did we get this right? 👍 / 👎 to inform future reviews.
| "prettier": "^2.0.5", | ||
| "react": "18.3.1", | ||
| "react-native": "0.77.1", | ||
| "react": "19.1.0", |
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 update from React 18.3.1 to 19.1.0 is a major version upgrade that introduces breaking changes. Ensure that all React 19 breaking changes have been reviewed and that the codebase is compatible. Specifically, React 19 has changes to refs, context, and hooks behavior that may require code updates beyond just dependency version bumps.
Did we get this right? 👍 / 👎 to inform future reviews.
| animationType="none" | ||
| hardwareAccelerated={false} | ||
| onRequestClose={[Function]} |
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 removal of the hardwareAccelerated={false} prop from Modal snapshots indicates a change in React Native's Modal component API between versions. While snapshot updates are expected with dependency upgrades, verify that this prop removal doesn't affect the actual runtime behavior of modals in the application, especially on Android where hardware acceleration can impact performance.
Did we get this right? 👍 / 👎 to inform future reviews.
f5710f4 to
c695a04
Compare
73bb9e1 to
4a57918
Compare
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 changes LGTM 🚀
Thank you for all your work on this @alwx 🙇
f8c3f6b to
32d6cd0
Compare
|
Looking good! the changed kotlin files from the test app should be linted, after that we could merge this PR! |
performance-tests/TestAppPlain/android/app/src/main/java/com/testappplain/MainApplication.kt
Outdated
Show resolved
Hide resolved
performance-tests/TestAppSentry/android/app/src/main/java/com/testappsentry/MainApplication.kt
Outdated
Show resolved
Hide resolved
95e360d to
b8689f4
Compare
📢 Type of change
📜 Description
After we've got rid of
react-test-renderer, nothing was blocking us from upgrading core to 0.80. Because some mocks were missing in 0.80.0 (facebook/react-native#51993), the update was done to 0.80.1 straight away.In addition to that, tests were updated (just because some of them required certain modifications after the RN version bump).
📝 Checklist
sendDefaultPIIis enabled#skip-changelog