Skip to content

Conversation

christophpurrer
Copy link

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

This aligns with the current iOS example:
https://github.com/microsoft/react-native-macos/blob/main/packages/rn-tester/RNTester/AppDelegate.mm#L45

Changelog

[macOS] [Added] - Add Fabric example to rn-tester-macOS

Test Plan

This code compiles and runs with other Fabric related changes which are still in flight or need to be pushed against this repo.
Here is my demo branch:
https://github.com/christophpurrer/react-native-macos/commits/road2fabric

@christophpurrer christophpurrer requested a review from a team as a code owner January 18, 2023 08:32
Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

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

Ok I checked, and as far back as 60-stable, RNTester iOS never had a file called ViewController.mm like we do. So my guess is that was a personal refactoring our team chose rather than putting everything in App delegate.mm

I guess as long as the isn't confusing, that's fine. I was wondering if we should try and change our template to be as close to the iOS one as possible.

@christophpurrer
Copy link
Author

I guess as long as the isn't confusing, that's fine. I was wondering if we should try and change our template to be as close to the iOS one as possible.

That is a desirable goal.
@shwanton already made some changes last year to bring 'missing' features from rn-tester-ios to rn-tester-macOS (Hermes).
With the new architecture approaching it would be great to align the iOS and macOS rn-tester apps as much as possible

@Saadnajmi Saadnajmi self-assigned this Jan 18, 2023
@Saadnajmi Saadnajmi merged commit cd63d4c into microsoft:main Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants