Skip to content
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

[RNMobile] Upgrade to RN 0.64 #29118

Merged
merged 251 commits into from
Jun 16, 2021
Merged

[RNMobile] Upgrade to RN 0.64 #29118

merged 251 commits into from
Jun 16, 2021

Conversation

dratwas
Copy link
Contributor

@dratwas dratwas commented Feb 18, 2021

Description

Upgrade React Native to version 0.64.0 (See RN Upgrade Helper)

Related PRs:

How has this been tested?

To test: Gutenberg Editor should work as expected.
Try to run all the manual tests.

Remaining Tasks:

  • rnmobile/try/upgrade-0-64 branch is updated to 0.64.0-rc3 and needs to be updated to 0.64.0 stable.
  • rnmobile/try/upgrade-0-64 needs to be updated with latest gutenberg trunk which contains some larger updates for handling WPAndroid composite builds
  • iOS Demo App needs to be fixed on rnmobile/try/upgrade-0-64
  • Need to create relevant PRs in other Repos following model of Upgrade React Native to 0.63 wordpress-mobile/gutenberg-mobile#2580
  • Re-target to Gutenberg Web React 17 update branch, or bring those changes in here
  • Check CI across all repos and address any issues

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

SergioEstevao and others added 30 commits April 28, 2020 10:43
Use accessibility label to find TextInput component instead of the class
name. The class name fails because the way TextInput is implemented in
0.62 where a ForwardRef is used.
Add isFocused method to mock of TextInput.
Changes after install on clean clone

change
# Conflicts:
#	package-lock.json
# Conflicts:
#	package-lock.json
# Conflicts:
#	package-lock.json
# Conflicts:
#	package-lock.json
#	packages/react-native-editor/ios/Podfile.lock
#	packages/react-native-editor/package.json
#	packages/scripts/scripts/check-licenses.js
# Conflicts:
#	packages/react-native-editor/ios/Podfile.lock
Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a look at the native mobile changes and look good to me! I've only left some non-blocking comments.

Also, I didn't particularly review the iOS side changes but I trust our (successful) testing so far for that. At this point, and since the upgrade effort is already monumental, takes long time and is hard to keep up-to-date, I'm fine with optimistically merging and we can fix any issues afterwards.

That said, since a few folks are doing a pass on the web-side of things (I see @gziolo's for example), let's wait for those review passes to conclude as well before merging.

@gziolo
Copy link
Member

gziolo commented Jun 15, 2021

From my perspective, the only concern for merging this PR is npm ci replaced in npm install in GitHub actions. It's something that we could change to move forward but we need to have a clear idea why it fails and how we can resolve it.

@youknowriad
Copy link
Contributor

From my perspective, the only concern for merging this PR is npm ci replaced in npm install in GitHub actions.

Can we just revert that and see, I think the main problem for these failures is independent from the upgrade.

@youknowriad
Copy link
Contributor

Note that the two failures we're seeing now (performance and post title) are unrelated for this PR and exists on trunk and we're working on them in #32686

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, assuming that npm ci works the same way as other PRs at the moment, this patch looks good to go.

Amazing teamwork everyone involved to get RN 0.64 and React 17 upgrades working 🎉 👏🏻

@ceyhun
Copy link
Member

ceyhun commented Jun 15, 2021

Note that the two failures we're seeing now (performance and post title) are unrelated for this PR and exists on trunk and we're working on them in #32686

Great to hear that 😅 I've pushed 2 commits (85c4331, e8cd4af) replacing npm install with npm ci to how they were before. Didn't see any cb() never called! errors, but I fear that might be related to ~/.npm cache still being in place and maybe the error will return when a new change to package-lock.json that makes the cache invalid. In any case I think we can go back to npm install or try to find another solution if that ever happens.

I'll get one final merge from trunk to fix remaining checks as #32686 seems to have been merged to trunk.

@ceyhun ceyhun merged commit b4623e3 into trunk Jun 16, 2021
@ceyhun ceyhun deleted the rnmobile/try/upgrade-0-64 branch June 16, 2021 17:49
@github-actions github-actions bot added this to the Gutenberg 10.9 milestone Jun 16, 2021
@youknowriad youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) Needs Dev Note Requires a developer note for a major WordPress release cycle
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.