Skip to content

Conversation

alloy
Copy link
Member

@alloy alloy commented Dec 21, 2019

  • I am making a fix / change for the macOS implementation of react-native

Part of #215

Summary

Updates podspecs to exclude iOS/tvOS source files and add macOS specific source files.

This is not a comprehensive pass through all the podspecs, rather just to get the RNTester macOS target up and running. We’ll likely need to go through it with a comb at some point and I’d also like two invert some of the file selections so we don’t need so much exclusion of files (but for now doing it this way will make it easier to sync from upstream).

Changelog

[macOS] [Added] - Add CocoaPods based RNTester target.

Test Plan

I have gone through half the samples to check if they work similarly to the product of the static Xcode project, I will continue to do so for the rest of the examples.

@alloy alloy added the enhancement New feature or request label Dec 21, 2019
@alloy alloy requested a review from acoates-ms as a code owner December 21, 2019 01:00
s.license = package["license"]
s.author = "Facebook, Inc. and its affiliates"
s.platforms = { :ios => "9.0", :tvos => "9.2" }
s.platforms = { :ios => "9.0", :tvos => "9.2", :osx => "10.14" }
Copy link
Member Author

Choose a reason for hiding this comment

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

This indicates a pod is available for a specific deployment target.

@alloy alloy mentioned this pull request Dec 21, 2019
pod 'DoubleConversion', :podspec => "#{prefix}/third-party-podspecs/DoubleConversion.podspec"
pod 'glog', :podspec => "#{prefix}/third-party-podspecs/glog.podspec"
pod 'Folly', :podspec => "#{prefix}/third-party-podspecs/Folly.podspec"
pod 'boost-for-react-native', :podspec => "#{prefix}/third-party-podspecs/boost-for-react-native.podspec"

Choose a reason for hiding this comment

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

Are we using the autolink-ios.rb file for Mac? Probably worth cleaning it up by making an autolink-apple.rb file that has everything shared between ios/mac and then autolink-ios and autolink-mac (you'll have to make this) would pull it in.

Then where we consume the autolink-ios.rb file right now we would just conditionally include the appropriate platform extension version based off the platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question; yes and no.

The “autolinking” that’s being referred to in this version of the Podfile is an earlier incarnation, the current version lives in react-native-community/cli. That one will definitely need updating for macOS, but I’ll do that in a follow-up PR [when I make a little RN macOS example app I have in mind].

As-is, the old version in this repo works for macOS too and is simply named autolinking-ios rather than actually only working for iOS.

@alloy alloy mentioned this pull request Jan 6, 2020
3 tasks
@ghost ghost removed the Needs: Author Feedback label Jan 6, 2020
@alloy alloy force-pushed the alloy/add-macos-cocoapods-rntester-target branch 3 times, most recently from b762e17 to c07f79e Compare January 7, 2020 16:10
@alloy
Copy link
Member Author

alloy commented Jan 7, 2020

@HeyImChris Ok, this should be good to go for another review pass. I did some more work around launching the macOS product.

alloy added 16 commits January 9, 2020 11:19
At least for now so we’re in sync with upstream v0.60.0
This change is already made upstream in commit
facebook@9ece5bda, so when that
is merged in this commit can be skipped.

Until then, not making this change will just lead to file thrashing.
This change is already made upstream in commit
facebook@c1845810, so when that
is merged in this commit can be skipped.

Until then, not making this change will just lead to file thrashing.
As per https://www.thecave.com/2015/08/10/dispatch-async-to-main-queue-doesnt-work-with-modal-window-on-mac-os-x/#update2
the main queue is already executing a block when we start the new
modal runloop and as the main queue is a serial queue it won’t perform
new work until the current block is finished. In short, the task is
queued, but will never be performed by GCD. This change side-steps GCD
and instead directly invokes -dismiss on the main thread.
@alloy alloy force-pushed the alloy/add-macos-cocoapods-rntester-target branch from c07f79e to b26a060 Compare January 9, 2020 10:24

platform :ios, '9.0'

require_relative '../scripts/autolink-ios'

Choose a reason for hiding this comment

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

Still seems wrong to pull in and use "autolink-ios" for both Mac and iOS, but given that we're trying to reduce diffs between us and facebook, it's probably not a blocking issue to get cleaned up right now. Ideally we'd have an autolink-apple, autolink-mac, and autolink-ios

Copy link
Member Author

Choose a reason for hiding this comment

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

Still seems wrong to pull in and use "autolink-ios" for both Mac and iOS, but given that we're trying to reduce diffs between us and facebook, it's probably not a blocking issue to get cleaned up right now. Ideally we'd have an autolink-apple, autolink-mac, and autolink-ios

Just in case this was missed before, this comment addresses the plan #211 (comment)

@alloy
Copy link
Member Author

alloy commented Jan 28, 2020

Just tried to merge this, but getting this error that I’ve never seen before. Googling for it doesn’t yield any results 🙁

Screenshot 2020-01-28 at 15 59 43

@alloy alloy merged commit 4a0ca8c into master Jan 28, 2020
@alloy alloy deleted the alloy/add-macos-cocoapods-rntester-target branch January 28, 2020 16:02
@alloy
Copy link
Member Author

alloy commented Jan 28, 2020

Oh, well, now it worked 🤷‍♂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants