-
Notifications
You must be signed in to change notification settings - Fork 583
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
build(deps): bump react-native from 0.72.17 to 0.73.9 #10739
Conversation
9615470
to
21b596e
Compare
@@ -1,3 +1,3 @@ | |||
nodejs 20.9.0 | |||
ruby 3.1.4 | |||
java zulu-11.58.15 | |||
java zulu-17.50.19 |
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.
Java bump to 17
@@ -0,0 +1,99 @@ | |||
package net.artsy.app |
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.
we had to re-write this with Kotlin 🙈
@@ -0,0 +1,86 @@ | |||
package net.artsy.app | |||
|
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.
we had to re-write this with Kotlin 🙈
extraNodeModules: { | ||
images: path.resolve(__dirname, "./images"), // Add this line for Metro to resolve 'images folder' | ||
}, |
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.
Big GOTCHA, images/
path stopped being recognised after the rn bump and we had to let metro config know where they are
"@react-native-async-storage/async-storage": "1.19.8", | ||
"@react-native-clipboard/clipboard": "1.14.1", |
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.
Had to bump these in order for them to be rn 73 compliant
"react-native-blob-util": "0.19.9", | ||
"react-native-blurhash": "1.1.11", | ||
"react-native-bootsplash": "3.2.0", | ||
"react-native-code-push": "8.1.0", | ||
"react-native-code-push": "8.3.1", |
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.
Had to bump in order for them to be rn 73 compliant
@@ -190,7 +191,7 @@ | |||
"react-native-url-polyfill": "2.0.0", | |||
"react-native-view-shot": "3.8.0", | |||
"react-native-vimeo-iframe": "1.2.1", | |||
"react-native-webview": "13.6.2", | |||
"react-native-webview": "13.6.3", |
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.
Had to bump in order for them to be rn 73 compliant
"react-native-config": "https://github.com/artsy/react-native-config.git#v1.4.12-artsy", | ||
"react-native-context-menu-view": "git+https://github.com/artsy/react-native-context-menu-view.git#v1.10.10-artsy", | ||
"react-native-dev-menu-android": "1.0.10", | ||
"react-native-device-info": "10.3.0", | ||
"react-native-document-picker": "9.3.0", | ||
"react-native-fast-image": "8.6.3", | ||
"react-native-fbsdk-next": "13.0.0", | ||
"react-native-gesture-handler": "2.13.1", | ||
"react-native-flipper": "0.265.0", | ||
"react-native-gesture-handler": "2.19.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.
Had to bump in order for them to be rn 73 compliant
3e57c3b
to
04199af
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.
This looks great!
Two small ones:
- the braze push handling in MainActivity is commented out, I think it is still necessary? 🤔
- I think the react native patch may have gotten some generated code carried along
#### Explanation/Context: | ||
|
||
We had to patch Braze in order to proceed with the react native upgrade to 0.73.9. The patch was needed to support kotlin jvm target 17 and | ||
in order to also make targetCompatibility and sourceCompatibility compatible with the JAVA 17 which is the standard for newer rn versions starting 0.73. |
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.
we should do some braze testing on Android in QA!
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.
💯
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.
we will probably need to test out both android & iOS thoroughly, had to bump to 8.4.0 in the end - will remove the hack entry
} | ||
val token = task.result | ||
// Log.i(TAG, "TOKEN firebase messaging token $token") | ||
// Braze.getInstance(applicationContext).setRegisteredPushToken(token) |
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.
this meant to still be commented out? do we need this to get push tokens registered for braze on Android?
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.
I was getting an error here when trying to build ~ I think that in the end we will need to bump braze to be compliant 🫨 .
Seems like they moved this function to JS side ~ will trigger betas after I address that to test if it works
Bumped braze to 8.4.0 in the end ~ moved the registration of the token to the JS side since I was getting build errors from a depracated method from Let me know if you have any concerns ~ but I Was thinking to open a separate PR for braze 🤷♂️ |
FreshBetas:
|
Braze works! Received notifications in both android / ios betas above |
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 pushing through this!! 💪 ⚡ 📈
@@ -235,6 +236,8 @@ export async function configure() { | |||
// (optional) Called when Token is generated (iOS and Android) | |||
onRegister: async (token) => { | |||
try { | |||
registerPushToken(token.token) |
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.
nice
759d3c6
759d3c6
to
c709193
Compare
1090bec
to
3a01245
Compare
This PR resolves PHIRE-1181
Description
Bumps react-native version from
0.72.17
to0.73.9
What changed:
You can read more info on RN 73 official blogpost
Which Deps were bumped and why:
@braze/react-native-sdk
:5.2.0
~>8.4.0
@react-native-async-storage/async-storage
:1.19.3
~>1.19.8
@react-native-clipboard/clipboard
:1.7.0
~>1.14.1
react-native-code-push
:8.1.0
~>8.3.1
react-native-gesture-handler
:2.13.1
~>2.19.0
react-native-webview
:13.6.2
~>13.6.3
React Native 0.73 will depend on Android Gradle Plugin (AGP) 8.x. This will require all the libraries to specify a namespace in their build.gradle file. RN team added a compatibility layer for libraries that haven't specified a namespace, but please consider updating your libraries nonetheless.
Read more here
Patches:
Enhanced react native patch with
.mock('../Libraries/Image/Image', () => mockComponent('../Libraries/Image/Image'))
due to this issue.DevExperience Before / After:
Metro is way faster when bundling the app (around ~2x faster)
Android builds have an insane boost that is mostly due to java 17 and gradle updates:
On emulator with cleared cache / deleted app:
on emulator with cached build:
Steps To Upgrade on your machine after this gets merged:
Afterwards in order to bump JAVA:
For asdf users:
For non-asdf users:
brew install --cask zulu@17 # Get path to where cask was installed to double-click installer brew info --cask zulu@17
Important
For all (asdf/ non asdf users). After you install the JDK, update your JAVA_HOME environment variable in your
.zshrc
file. Afterwards runsource ~/.zshrc
to make it effective and restart your terminal.If you used above steps, JDK will likely be at /Library/Java/JavaVirtualMachines/zulu-17.jdk/Contents/Home.
You can try out in the terminal by typing $JAVA_HOME to make sure that it was updated properly.
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.