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

Update layoutDirection in calls to ReactNativeIsland::Arrange #2248

Closed

Conversation

Yajur-Grover
Copy link
Contributor

Description

In Main.cpp, currently the call to ReactNativeIsland::Arrange() is being passed an 'undefined' layoutDirection. This change modifies the constraints object so that the layoutDirection is not undefined.

When trying to launch an example fabric app in this async-storage branch, I got the following error:
image

This change resolves this issue so that layoutDirection is never set to Undefined.

Platforms affected

  • Android
  • iOS
  • macOS
  • visionOS
  • Windows

Test plan

I initially tried following the below test plan, which I got from the PR adding New Architecture:

npm run set-react-version canary-windows
yarn
cd example
yarn install-windows-test-app --use-fabric
yarn windows

# In a separate terminal
yarn start

However, yarn windows would give me the following error:

"C:\new-account-repos\react-native-test-app\example\windows\Example.sln" (Restore target) (1) ->
(_FilterRestoreGraphProjectInputItems target) ->
  C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets(4
65,5): error MSB3202: The project file "C:\new-account-repos\react-native-test-app\example\Folly\Folly.vcxproj" was not
 found. [C:\new-account-repos\react-native-test-app\example\windows\Example.sln]
  C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets(4
65,5): error MSB3202: The project file "C:\new-account-repos\react-native-test-app\example\fmt\fmt.vcxproj" was not fou
nd. [C:\new-account-repos\react-native-test-app\example\windows\Example.sln]
  C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets(4
65,5): error MSB3202: The project file "C:\new-account-repos\react-native-test-app\example\Microsoft.ReactNative\Micros
oft.ReactNative.vcxproj" was not found. [C:\new-account-repos\react-native-test-app\example\windows\Example.sln]
  C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets(4
65,5): error MSB3202: The project file "C:\new-account-repos\react-native-test-app\example\Common\Common.vcxproj" was n
ot found. [C:\new-account-repos\react-native-test-app\example\windows\Example.sln]

Looking at the generated solution file Example.sln, the paths for several of the Project declarations were incorrect - e.g Microsoft.ReactNative had the path ..\\Microsoft.ReactNative\Microsoft.ReactNative.vcxproj whereas the correct path is ..\\..\node_modules\react-native-windows\Microsoft.ReactNative\Microsoft.ReactNative.vcxproj. After fixing all the instances of this path issue in Example.sln, the app was able to launch successfully.

@tido64
Copy link
Member

tido64 commented Sep 18, 2024

Can you check out #2249 and see if it fixes the paths in the solution file?

@Yajur-Grover
Copy link
Contributor Author

Can you check out #2249 and see if it fixes the paths in the solution file?

That change resolved the issues with the paths in the solution file.

@Yajur-Grover
Copy link
Contributor Author

@tido64 I ended up removing the web storage module which resolved the codegen-windows issues, and I can build the app successfully when running the app through Visual Studio:
image

However, when running yarn windows in example, I get the following error on the command line:

√ Found Solution: C:\new-account-repos\react-native-test-app\example\windows\Example.sln
i Build configuration: Debug
i Build platform: x64
× Building Solution: Package.appxmanifest : error APPX0703: Manifest references file 'Microsoft.ReactNative.dll'...
× Build failed with message 8:6>Package.appxmanifest : error APPX0703: Manifest references file 'Microsoft.ReactNative.dll' which is not part of the payload. [C:\new-account-repos\react-native-test-app\example\node_modules\.generated\windows\Win32\ReactApp.Package.wapproj]. Check your build configuration.
Command failed. Re-run the command with --logging for more information.

This appears to be the same error that you got in the comment here. Was wondering if you could repro the same on your end.

@Yajur-Grover Yajur-Grover marked this pull request as ready for review September 20, 2024 00:08
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert these changes?

winrt::Size size{window.ClientSize().Width / scaleFactor,
window.ClientSize().Height / scaleFactor};
rootView.Arrange({size, size, winrt::LayoutDirection::Undefined}, {0, 0});
constraints.MinimumSize = constraints.MaximumSize = size;
constraints.LayoutDirection = winrt::LayoutDirection::LeftToRight;
Copy link
Member

Choose a reason for hiding this comment

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

This is really odd. I looked at react-native-windows and cannot see LayoutDirection being set at all: https://github.com/search?q=repo%3Amicrosoft%2Freact-native-windows+UpdateRootViewSizeToAppWindow&type=code

How does this work upstream? I'm not sure hard-coding left-to-right is the correct thing to do here.

Copy link
Member

@tido64 tido64 Sep 20, 2024

Choose a reason for hiding this comment

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

@acoates-ms: What's the right fix here? It looks like the playground app hard-codes it to winrt::LayoutDirection::LeftToRight, but should it be read from somewhere?

@Yajur-Grover
Copy link
Contributor Author

Yajur-Grover commented Sep 24, 2024

@tido64 closing this for now as this PR fixes the logic in RNW when passing in undefined for layoutDirection.

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.

2 participants