Skip to content

Conversation

afoxman
Copy link
Contributor

@afoxman afoxman commented Nov 13, 2021

Platforms Impacted

  • iOS
  • macOS
  • win32 (Office)
  • windows
  • android

Description of changes

Update all packages to react-native 0.64. This requires that a number of related dependencies be updated as well, such as react-native-windows, react-native-macos, @office-iss/react-native-win32, react, several @types packages, etc.

I also added resolutions to force the use of Metro 0.66.2 or later, as that is required for certain @rnx-kit package updates. Metro 0.66.2 is compatible with react-native 0.64.

Verification

I ran a CI build locally, which ran automated tests and bundling. I will also run the tester app manually and inspect the UI.

Pull request checklist

This PR has considered (when applicable):

  • Automated Tests
  • Documentation and examples
  • Keyboard Accessibility
  • Voiceover
  • Internationalization and Right-to-left Layouts

}
}
accessible={true}
collapsable={false}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to self: check it we should enable this prop for performance gains. I vaguely recall this might do something similar to compose, where less views are in the render tree.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, also similar to react fragments

@afoxman
Copy link
Contributor Author

afoxman commented Nov 15, 2021

My next guess is that we may need to bump @react-native-community/cli. The iOS PR is running react-native run-ios, which I remember failed with RNTA & RN 0.64 until we made some upstream CLI fixes, which showed up in @react-native-community/cli version 5.02 microsoft/react-native-test-app#375

Looks like 5.0.2 was only for cli-platform-ios, and once I forced that version using a resolution, things started working on my dev machine. Thank you for the pointer! I was going crazy trying to understand this. I don't know why they didn't do a 5.0.2 release of the entire CLI.

@afoxman afoxman marked this pull request as ready for review November 15, 2021 18:21
@afoxman
Copy link
Contributor Author

afoxman commented Nov 15, 2021

My next guess is that we may need to bump @react-native-community/cli. The iOS PR is running react-native run-ios, which I remember failed with RNTA & RN 0.64 until we made some upstream CLI fixes, which showed up in @react-native-community/cli version 5.02 microsoft/react-native-test-app#375

Looks like 5.0.2 was only for cli-platform-ios, and once I forced that version using a resolution, things started working on my dev machine. Thank you for the pointer! I was going crazy trying to understand this. I don't know why they didn't do a 5.0.2 release of the entire CLI.

Turns out I wasn't running the right command and didn't see that the failure still occurred. Then I took a closer look at yarn.lock and saw that cli-platform-ios was already 5.0.2 even though CLI was 5.0.1. I tried forcing the CLI ^6.0.0 and that still doesn't resolve it.

So this hasn't actually been fixed in the CLI, in 5.x or 6.x.

@rurikoaraki
Copy link
Collaborator

@afoxman Did some testing locally for win32, things seem to look good. At some point we probably want to bump the build of rex to a build that includes the changes to office to go to 0.64, but that can be its own PR.

@Saadnajmi
Copy link
Collaborator

Saadnajmi commented Nov 16, 2021

My next guess is that we may need to bump @react-native-community/cli. The iOS PR is running react-native run-ios, which I remember failed with RNTA & RN 0.64 until we made some upstream CLI fixes, which showed up in @react-native-community/cli version 5.02 microsoft/react-native-test-app#375

Looks like 5.0.2 was only for cli-platform-ios, and once I forced that version using a resolution, things started working on my dev machine. Thank you for the pointer! I was going crazy trying to understand this. I don't know why they didn't do a 5.0.2 release of the entire CLI.

Turns out I wasn't running the right command and didn't see that the failure still occurred. Then I took a closer look at yarn.lock and saw that cli-platform-ios was already 5.0.2 even though CLI was 5.0.1. I tried forcing the CLI ^6.0.0 and that still doesn't resolve it.

So this hasn't actually been fixed in the CLI, in 5.x or 6.x.

@tido64 FYI it seems your CLI fix didn't make it into later releases =/

@tido64
Copy link
Member

tido64 commented Nov 16, 2021

Is run-ios failing? I can have a look later…

@Saadnajmi
Copy link
Collaborator

Saadnajmi commented Nov 16, 2021

Is run-ios failing? I can have a look later…

@afoxman worked around it by copying the fix RNTA had to the iOS test app (which as I type, just realized is what running yarn configure-test-app would have done). So run-is is fine.

The reason for tagging you was simply to say "cli-platform-ios v 5.0.2" (which has your fix) isn't included in either the 5.x or 6.x release of "@react-native-community/cli"

@tido64
Copy link
Member

tido64 commented Nov 16, 2021

The reason for tagging you was simply to say "cli-platform-ios v 5.0.2" (which has your fix) isn't included in either the 5.x or 6.x release of "@react-native-community/cli"

The fix I did was in cli-platform-ios though. It wouldn't be in @react-native-community/cli. But installing it should've pulled down latest cli-platform-ios.

I did manage to run-ios successfully:

% yarn ios
yarn run v1.22.17
$ react-native run-ios --scheme ReactTestApp --project-path src
error React Native CLI uses autolinking for native dependencies, but the following modules are linked manually:
  - @react-native-community/slider (to unlink run: "react-native unlink @react-native-community/slider")
  - react-native-svg (to unlink run: "react-native unlink react-native-svg")
This is likely happening when upgrading React Native from below 0.60 to 0.60 or above. Going forward, you can unlink this dependency via "react-native unlink <dependency>" and it will be included in your app automatically. If a library isn't compatible with autolinking, disregard this message and notify the library maintainers.
Read more about autolinking: https://github.com/react-native-community/cli/blob/master/docs/autolinking.md
info Found Xcode workspace "FluentTester.xcworkspace"
info Launching iPhone 12 (iOS 14.5)
info Building (using "xcodebuild -workspace FluentTester.xcworkspace -configuration Debug -scheme ReactTestApp -destination id=55FCD6EA-B6C0-4491-ABE9-074D19045567")
success Successfully built the app
info Installing "/~/Library/Developer/Xcode/DerivedData/FluentTester-genvaxlzkxuvwdegafenhwouwpil/Build/Products/Debug-iphonesimulator/ReactTestApp.app"
info Launching "com.microsoft.ReactTestApp"
success Successfully launched the app on the simulator
✨  Done in 129.27s.

We should probably remove @react-native-community/slider and react-native-svg from ios/Podfile. They should be autolinked already.

Edit: In general, you should add the packages you want to link as dependencies (or devDependencies) and autolink will pick them up.

@Saadnajmi
Copy link
Collaborator

The reason for tagging you was simply to say "cli-platform-ios v 5.0.2" (which has your fix) isn't included in either the 5.x or 6.x release of "@react-native-community/cli"

The fix I did was in cli-platform-ios though. It wouldn't be in @react-native-community/cli. But installing it should've pulled down latest cli-platform-ios.

I did manage to run-ios successfully:

% yarn ios
yarn run v1.22.17
$ react-native run-ios --scheme ReactTestApp --project-path src
error React Native CLI uses autolinking for native dependencies, but the following modules are linked manually:
  - @react-native-community/slider (to unlink run: "react-native unlink @react-native-community/slider")
  - react-native-svg (to unlink run: "react-native unlink react-native-svg")
This is likely happening when upgrading React Native from below 0.60 to 0.60 or above. Going forward, you can unlink this dependency via "react-native unlink <dependency>" and it will be included in your app automatically. If a library isn't compatible with autolinking, disregard this message and notify the library maintainers.
Read more about autolinking: https://github.com/react-native-community/cli/blob/master/docs/autolinking.md
info Found Xcode workspace "FluentTester.xcworkspace"
info Launching iPhone 12 (iOS 14.5)
info Building (using "xcodebuild -workspace FluentTester.xcworkspace -configuration Debug -scheme ReactTestApp -destination id=55FCD6EA-B6C0-4491-ABE9-074D19045567")
success Successfully built the app
info Installing "/~/Library/Developer/Xcode/DerivedData/FluentTester-genvaxlzkxuvwdegafenhwouwpil/Build/Products/Debug-iphonesimulator/ReactTestApp.app"
info Launching "com.microsoft.ReactTestApp"
success Successfully launched the app on the simulator
✨  Done in 129.27s.

We should probably remove @react-native-community/slider and react-native-svg from ios/Podfile. They should be autolinked already.

Edit: In general, you should add the packages you want to link as dependencies (or devDependencies) and autolink will pick them up.

Good callout on removing slider & SVG, since those are direct dependencies and can get autolinked. I can look into that separately since that doesn't seem to be blocking (Having issues filing an issue with GitHub to track right now).

Interestingly enough, cli-platform-ios comes in as a dependency of react-native, not @react-native-community/cli. So bumping the cli package didn't also bump cli-platform-ios. Nonetheless, the run-ios issue was resolved since it was fixed in RNTA and made backwards compatible.

@Saadnajmi
Copy link
Collaborator

Also tagging @lenahong who I think is most recently involved with FURN Android stuff. This PR has the Android CI failing, and I'm still not sure why.

@afoxman
Copy link
Contributor Author

afoxman commented Nov 17, 2021

Also tagging @lenahong who I think is most recently involved with FURN Android stuff. This PR has the Android CI failing, and I'm still not sure why.

I was able to make this pass locally by changing @react-native-community/slider's gradle.properties:

ReactNativeSlider_minSdkVersion=21  // was 16

I don't know enough Gradle yet to understand if/how I can change this from the android app.

@Saadnajmi @lenahong

@tido64
Copy link
Member

tido64 commented Nov 17, 2021

Can you try setting minSdkVersion in /apps/android/src/build.gradle:

diff --git a/apps/android/src/build.gradle b/apps/android/src/build.gradle
index c9a77b9e..45595823 100644
--- a/apps/android/src/build.gradle
+++ b/apps/android/src/build.gradle
@@ -28,3 +28,6 @@ allprojects {
         jcenter()
     }
 }
+ext {
+    minSdkVersion = 21
+}

We should also align on the Flipper version that works with 0.64:

diff --git a/apps/android/src/gradle.properties b/apps/android/src/gradle.properties
index 1bbc8cc2..d21d03f2 100644
--- a/apps/android/src/gradle.properties
+++ b/apps/android/src/gradle.properties
@@ -25,4 +25,4 @@ android.useAndroidX=true
 android.enableJetifier=true

 # Version of flipper SDK to use with React Native
-FLIPPER_VERSION=0.33.1
+FLIPPER_VERSION=0.75.1

Edit: I'll see if we can't set minSdkVersion in RNTA. Please try bumping react-native-test-app to 0.9.13.

@afoxman
Copy link
Contributor Author

afoxman commented Nov 17, 2021

Can you try setting minSdkVersion in /apps/android/src/build.gradle:

diff --git a/apps/android/src/build.gradle b/apps/android/src/build.gradle
index c9a77b9e..45595823 100644
--- a/apps/android/src/build.gradle
+++ b/apps/android/src/build.gradle
@@ -28,3 +28,6 @@ allprojects {
         jcenter()
     }
 }
+ext {
+    minSdkVersion = 21
+}

Thanks! I tried this, and got a gradle error:

* Where:
Build file '/.../fluentui-react-native/apps/android/src/build.gradle' line: 31

* What went wrong:
A problem occurred evaluating root project 'FluentTester'.
> No signature of method: build_6i2c3eh3t4fr2egx4gv4xj8sx.ext() is applicable for argument types: (build_6i2c3eh3t4fr2egx4gv4xj8sx$_run_closure2) values: [build_6i2c3eh3t4fr2egx4gv4xj8sx$_run_closure2@79f65395]
  Possible solutions: exec(groovy.lang.Closure), exec(org.gradle.api.Action), wait(), run(), run(), any()

@tido64
Copy link
Member

tido64 commented Nov 17, 2021

Thanks! I tried this, and got a gradle error:

That's odd. I tested this locally and it works. Can you try bumping to latest react-native-test-app instead?

@afoxman
Copy link
Contributor Author

afoxman commented Nov 17, 2021

Thanks! I tried this, and got a gradle error:

That's odd. I tested this locally and it works. Can you try bumping to latest react-native-test-app instead?

Updating RNTA fixed it locally so I've pushed the change. CI runs things a bit differently - not easy to match/reproduce, which is a problem in itself, so I'll keep an eye on it.

@afoxman afoxman merged commit f16d742 into microsoft:master Nov 17, 2021
@afoxman afoxman deleted the rn64 branch November 17, 2021 19:11
@Saadnajmi Saadnajmi mentioned this pull request Jul 11, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants