Skip to content

Conversation

@Saadnajmi
Copy link
Collaborator

@Saadnajmi Saadnajmi commented Mar 23, 2022

Platforms Impacted

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

Description of changes

We have separate test apps for each platform, with a shared JS package as source.

apps
├── fluent-tester (shared JS)
├── android
├── ios
├── macos
└── windows

This is a weird structure that nobody else has. It's a relic of when I set up the test apps and was still learning React Native.
Most just put everything in one package:

apps
├── fluent-tester (shared JS + platform native code)

Now that iOS, Android, macOS, and Windows are all managed by react-native-test-app, let's just combine them. There are a lot of advantages of maintainability with having one package:

  • Less ability to have duplicate dependencies (like separate versions of React Native)
  • Easier to update with React Native version upgrades
  • Less code overall

Verification

Locally, I can run the Android, iOS, macOS, and windows test apps. All the CI is passing, and E2E tests should be running for windows & win32.

Pull request checklist

This PR has considered (when applicable):

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

@Saadnajmi
Copy link
Collaborator Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@Saadnajmi
Copy link
Collaborator Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@Saadnajmi Saadnajmi marked this pull request as ready for review March 24, 2022 20:25
@Saadnajmi Saadnajmi requested a review from a team as a code owner March 24, 2022 20:25
@Saadnajmi Saadnajmi changed the title [Draft] Combine platform specific test apps Combine platform specific test apps (except win32) Mar 24, 2022
Copy link
Member

@tido64 tido64 left a comment

Choose a reason for hiding this comment

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

On Windows, I'm seeing two issues. One is:

Screenshot 2022-07-22 at 11 36 12

Which should be fixed by removing FlatList since you don't need it here anyway:

diff --git a/apps/fluent-tester/src/TestComponents/Tokens/TokenTest.tsx b/apps/fluent-tester/src/TestComponents/Tokens/TokenTest.tsx
index 5267d9050..b41844bb9 100644
--- a/apps/fluent-tester/src/TestComponents/Tokens/TokenTest.tsx
+++ b/apps/fluent-tester/src/TestComponents/Tokens/TokenTest.tsx
@@ -1,5 +1,5 @@
 import * as React from 'react';
-import { FlatList, View, ViewStyle, StyleSheet, ColorValue } from 'react-native';
+import { ColorValue, ScrollView, StyleSheet, View, ViewStyle } from 'react-native';
 import { useTheme, Theme } from '@fluentui-react-native/theme-types';
 import { themedStyleSheet } from '@fluentui-react-native/themed-stylesheet';
 import { getCurrentAppearance } from '@fluentui-react-native/theming-utils';
@@ -88,7 +88,7 @@ const AliasTokensSwatchList: React.FunctionComponent = () => {
     <View style={[commonTestStyles.view]}>
       <Text>Alias Color Tokens from Token Pipeline</Text>
-      <View style={themedStyles.stackStyle}>
-        <FlatList data={aliasTokensAsArray} renderItem={renderSwatch} />
-      </View>
+      <View style={themedStyles.stackStyle}>{aliasTokensAsArray.map((item) => renderSwatch({ item }))}</View>
       </View>
     </View>
   );

You probably have to apply the same fix in ThemeTest.tsx as well.

The other is a crash if you click on anything in the sidebar. This is what's causing the Windows tests to fail. I tried to debug in Visual Studio and I see that a null command is passed to a node:

Screenshot 2022-07-22 at 12 07 13

I don't know why this starts to happen now though. We should ask someone with more intimate knowledge of react-native-windows.

Update: I've traced this to useViewCommandFocus. Does this hook support Windows? We are calling UIManager.dispatchViewManagerCommand with null because UIManager.getViewManagerConfig('RCTView').Commands is empty. Looking at react-native-windows/Libraries/Components/View/ReactNativeViewViewConfig.windows.js, it seems like it's supposed to be empty. Adding a check in useViewCommandFocus fixes the crash:

diff --git a/packages/utils/interactive-hooks/src/useViewCommandFocus.ts b/packages/utils/interactive-hooks/src/useViewCommandFocus.ts
index 40a7455d6..e7f2dc1e1 100644
--- a/packages/utils/interactive-hooks/src/useViewCommandFocus.ts
+++ b/packages/utils/interactive-hooks/src/useViewCommandFocus.ts
@@ -28,9 +28,10 @@ export function useViewCommandFocus(
       /**
        * Add focus() as a callable function to the forwarded reference.
        */
-      if (localRef) {
+      const focusCommand = UIManager.getViewManagerConfig('RCTView').Commands.focus;
+      if (localRef && focusCommand) {
         localRef.focus = () => {
-          UIManager.dispatchViewManagerCommand(findNodeHandle(localRef), UIManager.getViewManagerConfig('RCTView').Commands.focus, null);
+          UIManager.dispatchViewManagerCommand(findNodeHandle(localRef), focusCommand, null);
         };
       }
     },

Alternatively, pass in 'focus' explicitly. yarn e2etest:windows passes locally after these changes.

image

Update 2: I figured out why you're seeing this crash now. In the previous setup, we used RNW NuGet packages:

"prewindows": "install-windows-test-app --use-nuget",

In this PR, you changed to consume source instead:

yarn install-windows-test-app

The reason this is significant is because the "crash" we're seeing is actually an assert: https://github.com/microsoft/react-native-windows/blob/7ce4107c9e3c39119b8870805cc674f83af7a306/vnext/Microsoft.ReactNative/Views/ViewManagerBase.cpp#L80

And asserts are compiled away in release builds (i.e. NuGet packages).

displayName: Android PR
pool:
vmImage: 'windows-2019'
vmImage: 'macos-11'
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we using macos11? so old and it might not have all the latest security stuff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is probably a relic from pre EO work. Can update.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turns out the Android CI hangs without this. Bringing this back and I'll file an issue for later :/

Saadnajmi and others added 5 commits July 22, 2022 12:44
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Copy link
Member

@tido64 tido64 left a comment

Choose a reason for hiding this comment

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

Awesome job 🎉

@Saadnajmi Saadnajmi merged commit 0772c56 into microsoft:main Jul 25, 2022
@Saadnajmi Saadnajmi mentioned this pull request Aug 5, 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