feat(cli-platform-ios): add macOS auto-linking support #1126

alloy · 2020-04-22T16:14:12Z

Summary:

These changes make it so react-native-macos apps can use the native_modules.rb script as-is, but only activate pods in those targets that target the platform the pods support.

For instance, consider these npm packages that come with native modules:

ios-dep
macos-dep
ios-and-macos-dep

…and given a Podfile like the following:

require_relative '../node_modules/@react-native-community/cli-platform-ios/native_modules'

abstract_target 'Shared' do
  pod 'React', :path => "../node_modules/react-native-macos/"
  # ...

  target 'A iOS Target' do
    platform :ios, '9'
    use_native_modules!
  end

  target 'A macOS Target' do
    platform :macos, '10.14'
    use_native_modules!
  end
end

…the result would be that:

A iOS Target has the following dependencies:
- ios-dep
- ios-and-macos-dep
A macOS Target has the following dependencies:
- macos-dep
- ios-and-macos-dep

NOTE: In a future major version, the native_modules support will likely move to a cli-platform-apple package, but for now this was the least invasive change.

Test Plan:

Changes have full test coverage, which have been moved from Ruby to JS, and I’ve performed test runs with new applications.

alloy · 2020-04-22T16:17:14Z

packages/cli-types/src/index.ts

 * @property commands - An array of commands that are present in 3rd party packages
 */
-export type Config = {
+export interface Config extends IOSNativeModulesConfig {


This is done purely to ensure the Config doesn’t deviate in a way that the otherwise uncheckable Ruby source.

alloy · 2020-04-22T16:17:52Z

packages/cli-types/src/ios.ts

+   *             have the name of the package. This property will be
+   *             removed in a future major version.
+   *
+   * @todo Log a warning when this is used.


Have to follow-up on this in a future iteration.

We'll need to identify libraries that follow a pattern of adding a Podspec into ios/ dir and send PRs.

alloy · 2020-04-22T16:19:07Z

packages/cli-types/src/ios.ts

+ * only make changes to this interface (or `IOSScriptPhase`) if the data
+ * requirements of `native_modules.rb` change.
+ */
+export interface IOSNativeModulesConfig {


❗ So this should only change if the Ruby source changes and should not be refactored/shared with other typings, even if they are similar.

Why is that? Wouldn't it be better to just reference

cli/packages/cli-types/src/index.ts

Lines 131 to 153 in f570baf

export interface Config extends IOSNativeModulesConfig {

root: string;

reactNativePath: string;

project: ProjectConfig;

assets: string[];

dependencies: {[key: string]: Dependency};

platforms: {

android: PlatformConfig<

AndroidProjectConfig,

AndroidProjectParams,

AndroidDependencyConfig,

AndroidDependencyParams

>;

ios: PlatformConfig<

IOSProjectConfig,

IOSProjectParams,

IOSDependencyConfig,

IOSDependencyParams

>;

[name: string]: PlatformConfig<any, any, any, any>;

};

commands: Command[];

}

directly? This is the exact type of the JSON output from the command.

Could you help me understand where and how it is used? Maybe it's not the right place for it then (in shared package), but next to a utility?

Because TS cannot type-check the Ruby code, so people making changes to this Config interface might accidentally break some input data assumption made by it. Put differently, you should see this interface as if it were the function signature declaration of the Ruby code, if it were written in TS.

Only include pods in targets that target platforms the pod supports.

alloy · 2020-04-22T16:26:40Z

packages/platform-ios/native_modules.rb


+    # Skip pods that do not support the platform of the current target.
+    if platform = current_target_definition.platform
+      next unless spec.supported_on_platform?(platform.name)


This is the actual semantic change of the PR, so tiny 😂

Thanks for that comment! And also, I am impressed by the CocoaPods API :D

alloy · 2020-04-22T16:27:08Z

packages/platform-ios/native_modules.rb

-        use_native_modules!(config)
-      end
+  require "json"
+  runInput = JSON.parse(ARGF.read)


The Ruby script receives a JSON payload via stdin…

alloy · 2020-04-22T16:27:33Z

packages/platform-ios/native_modules.rb

-    end
+  unless runInput["capture_stdout"]
+    puts podfile.to_hash.to_json
  end


…and spits out a serialised version of the Podfile to stdout.

thymikee · 2020-04-22T16:34:28Z

.circleci/config.yml

      - run-lint
-  cocoa-pods:
+  unit-tests:
    executor: noderuby


Do we know which version of Node is run on this image? Would be cool if it was the latest or so

I couldn’t spot an explicit listing of the version in the CI output, but it’s using circleci/ruby:2.4-node and in this listing I see ENV NODE_VERSION=12.16.2. So probably that?

alloy · 2020-04-22T17:17:31Z

Seeing as it’s failing on the lts-tests, and also not to make things too hard on collaborators, I’ll add a check and disable the tests in case the env doesn’t support ruby/cocoapods.

…linking See react-native-community/cli#1126

…linking (#318) See react-native-community/cli#1126

grabbou

Like the changes. Good job! Just not sure about typings itself, rather than that, let's ship it!

alloy · 2020-05-27T10:04:39Z

Ace, thanks for the review! I feel confident about the need for the typing, but if needed we can amend the wording in the comment to clarify in a follow-up PR, so I’ll go ahead and merge for now 👍

alloy added 4 commits April 21, 2020 21:23

[platform-ios] Describe native_modules.rb input

515671a

[package] Add native_modules.rb to jest suite

58802c3

[platform-ios/native_modules] Use CP API in tests

c595ef5

[platform-ios/native_modules] Port tests to JS

81fb8ef

alloy requested review from Esemesek, grabbou and thymikee as code owners April 22, 2020 16:14

alloy commented Apr 22, 2020

View reviewed changes

[platform-ios/native_modules] Add platform specificity

9ef86dd

Only include pods in targets that target platforms the pod supports.

alloy force-pushed the add-macos-autolinking-to-cli-platform-ios branch from 85040b6 to 9ef86dd Compare April 22, 2020 16:26

alloy commented Apr 22, 2020

View reviewed changes

thymikee reviewed Apr 22, 2020

View reviewed changes

[platform-ios/native_modules] Disable tests in unssuported envs

1a411b3

alloy force-pushed the add-macos-autolinking-to-cli-platform-ios branch from e5382db to 1a411b3 Compare April 22, 2020 17:57

alloy added 2 commits April 23, 2020 11:19

[platform-ios/native_modules] Clean-up test

430c4ba

[platform-ios/native_modules] Improve output

f570baf

alloy added a commit to microsoft/react-native-macos that referenced this pull request Apr 23, 2020

[generator-macos] Setup Podfile for platform specificity during auto-…

0f6e9e6

…linking See react-native-community/cli#1126

alloy mentioned this pull request Apr 23, 2020

[generator-macos] Update template for platform specificity during auto-linking microsoft/react-native-macos#318

Merged

4 tasks

alloy added a commit to microsoft/react-native-macos that referenced this pull request Apr 23, 2020

[generator-macos] Setup Podfile for platform specificity during auto-…

8cb99cd

…linking (#318) See react-native-community/cli#1126

alloy mentioned this pull request May 25, 2020

Cannot ignore auto linking of un-supported library microsoft/react-native-macos#414

Closed

grabbou approved these changes May 26, 2020

View reviewed changes

alloy merged commit 0a93be1 into react-native-community:master May 27, 2020

alloy deleted the add-macos-autolinking-to-cli-platform-ios branch May 27, 2020 10:06

alloy mentioned this pull request May 27, 2020

Ability to skip autolinking for dependency for specific ios target #1179

Closed

	export interface Config extends IOSNativeModulesConfig {
	root: string;
	reactNativePath: string;
	project: ProjectConfig;
	assets: string[];
	dependencies: {[key: string]: Dependency};
	platforms: {
	android: PlatformConfig<
	AndroidProjectConfig,
	AndroidProjectParams,
	AndroidDependencyConfig,
	AndroidDependencyParams
	>;
	ios: PlatformConfig<
	IOSProjectConfig,
	IOSProjectParams,
	IOSDependencyConfig,
	IOSDependencyParams
	>;
	[name: string]: PlatformConfig<any, any, any, any>;
	};
	commands: Command[];
	}

feat(cli-platform-ios): add macOS auto-linking support #1126

feat(cli-platform-ios): add macOS auto-linking support #1126

Uh oh!

Conversation

alloy commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Test Plan:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alloy commented Apr 22, 2020

Uh oh!

grabbou left a comment

Choose a reason for hiding this comment

Uh oh!

alloy commented May 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alloy commented Apr 22, 2020 •

edited

Loading