-
Notifications
You must be signed in to change notification settings - Fork 58
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
React Native 0.69.4 Upgrade #5193
Conversation
Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job! |
@@ -54,7 +54,7 @@ | |||
"metro-resolver": "^0.70.3" | |||
}, | |||
"scripts": { | |||
"postinstall": "patch-package && npm ci --prefix gutenberg && npm run i18n:check-cache && ./bin/install-jetpack.sh", | |||
"postinstall": "patch-package && npm run clean:gutenberg:distclean && npm ci --prefix gutenberg && npm run i18n:check-cache && ./bin/install-jetpack.sh", |
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'm wondering if it's necessary cleaning up Gutenberg on every dependency install 🤔. I noticed in the package-lock.json
that the react
dependency has been updated to 18 (reference), shouldn't that be enough to install the correct versions of the dependencies under Gutenberg?
My main concern about this workaround is that I understand that it will increase severely the duration of the dependencies install command, do we know roughly how much time we expect that to be increased?
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.
Hey there, sorry for taking a bit to reply to this comment 😅
shouldn't that be enough to install the correct versions of the dependencies under Gutenberg?
It looks like we have other dependencies that still use React 17 and it's causing conflicts with the dependencies and the editor.
My main concern about this workaround is that I understand that it will increase severely the duration of the dependencies install command, do we know roughly how much time we expect that to be increased?
What do you mean by: it will increase severely? That new step we are adding it's just removing any dependencies that might have been installed in Gutenberg when we run npm install
in Gutenberg mobile
before running npm ci
. It's just a few seconds while it removes all of the node_module
folders within Gutenberg
that might have com from running npm install
from the parent folder. From all of the tests I've done while working with the upgrade, it doesn't take more than a few seconds.
We could add a follow-up task to investigate further the dependencies we have to avoid this workaround.
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.
It looks like we have other dependencies that still use React 17 and it's causing conflicts with the dependencies and the editor.
Ah, interesting. Do we know which dependencies are still using React 17?
On the other hand, do we need to run npm run clean:gutenberg:distclean
everytime or is this a one-off?
What do you mean by: it will increase severely? That new step we are adding it's just removing any dependencies that might have been installed in Gutenberg when we run npm install in Gutenberg mobile before running npm ci. It's just a few seconds while it removes all of the node_module folders within Gutenberg that might have com from running npm install from the parent folder. From all of the tests I've done while working with the upgrade, it doesn't take more than a few seconds.
Ah, ok, my concern came because I thought that cleaning Gutenberg would increase the duration of the npm ci --prefix gutenberg
command. In case it's just adding a few seconds then it's ok to continue with this workaround. Thanks for elaborating on this 🙇 !
We could add a follow-up task to investigate further the dependencies we have to avoid this workaround.
This would be great! Following on your comment regarding the fact that the duration won't be increased, I understand that is not a high priority, but having a ticket as a follow-up would be helpful.
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.
Ah, interesting. Do we know which dependencies are still using React 17?
I saw a few but can't remember from when I tested, I'll add this in the ticket as well so we have a list.
On the other hand, do we need to run npm run clean:gutenberg:distclean everytime or is this a one-off?
Just when an npm install
happens, I know it's not ideal but we can work on this after this gets merged.
This would be great! Following on your comment regarding the fact that the duration won't be increased, I understand that is not a high priority, but having a ticket as a follow-up would be helpful.
I mean it will be increased by a few seconds but it won't impact our development process or CI so much. But still, I'll create a ticket to investigate this a bit further.
6e7d1ec
to
c7bb61b
Compare
dc45609
to
15470f5
Compare
# Conflicts: # bundle/ios/App.js # bundle/ios/App.js.map # gutenberg
FYI some CI checks will fail until this PR WordPress/gutenberg#46220 gets merged. |
Heads up that WordPress/gutenberg#46220 is already merged in |
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.
LGTM! 🚀
@geriux @derekblank please when you're available it would be great to include this change in the release notes. I don't think we should block the merge due to this, especially as we'd need to have |
This PR adds support for React Native
0.69.4
The main changes are:
bin/generate-podspecs.sh
to support the new changes like Hermes being bundled with the React Native library.bundle:android:bytecode
since the path for Hermes changed.clean:gutenberg:distclean
postinstall script to clean upnode_modules
in Gutenberg since it was generating a conflict issue with multiple versions of React. There will be a follow-up ticket to investigate this further so we can remove this extra step.Related PRs
Feature branch
Android integration
iOS integration
To test:
PR submission checklist: