Skip to content

Get react-native building with Xcode 12 universal#504

Merged
HeyImChris merged 4 commits intomicrosoft:masterfrom
HeyImChris:xcodeUniversalBuild
Jul 14, 2020
Merged

Get react-native building with Xcode 12 universal#504
HeyImChris merged 4 commits intomicrosoft:masterfrom
HeyImChris:xcodeUniversalBuild

Conversation

@HeyImChris
Copy link
Copy Markdown

@HeyImChris HeyImChris commented Jul 14, 2020

Please select one of the following

  • 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

Apple's new Silicon based machines require to be built with arm64. A precursor to that is we need to get building with Xcode 12 Universal. I built RN using Xcode 12 Universal Beta 2 and this fixes up the new errors.

All the errors I hit were around the font property on RCTPicker being invalid. We're overriding NSControl's font property but with stricter checking in Xcode 12 universal, we can't do this anymore. Instead, let's just use the NSControl prop on macOS and iOS and define the prop ourselves on iOS when we need to. This also has the benefit of reducing the amount of UIFont/NSFont macro calls.

Microsoft Reviewers: Open in CodeFlow

@HeyImChris HeyImChris requested a review from Saadnajmi July 14, 2020 16:00
@HeyImChris HeyImChris requested a review from tom-un as a code owner July 14, 2020 16:00
@HeyImChris HeyImChris self-assigned this Jul 14, 2020
@pull-bot
Copy link
Copy Markdown

Messages
📖

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

DetailsCATEGORY may be:
  • General
  • macOS
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

📖 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 064a177

@property (nonatomic, assign) NSInteger selectedIndex;

@property (nonatomic, strong) RCTUIColor *color; // TODO(OSS Candidate ISS#2710739)
#if !TARGET_OS_OSX // [TODO(OSS Candidate ISS#2710739) // NSControl defines font prop for macOS, but iOS superviews don't
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For my own sake: Where can I see the list of ISS bugs / where they are filed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

There's an Issues tab at the top of the repo that shows them all https://github.com/microsoft/react-native-macos/issues

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is a pretty generic bug we use to track diffs in our fork and actually come to think of it, these should all probably get fixed up with GitHub bugs instead of ADO ones

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah I think we should stop attributing to the ADO one for sure - attributing to one single item doesn't have too much value and doesn't help during merges at all since if you're using a proper 3-way merge you don't need the additional context, your merge tool already will tell you what you've changed locally vs. what got changed in the other branch. I would vote towards not using these tags unless we can tag it with a github issue that is specific and maybe a good "first issue" candidate or we add comments to explain the divergence

@HeyImChris HeyImChris requested a review from amgleitman July 14, 2020 17:39
@HeyImChris HeyImChris merged commit d7ec297 into microsoft:master Jul 14, 2020
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.

4 participants