Skip to content

Conversation

tom-un
Copy link
Collaborator

@tom-un tom-un commented Jan 24, 2020

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native 👍
  • 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

When react-native-macos is installed alongside react-native in a repo, the react-native CLI scripts will discover both installations and run commands twice. The react-native.config.js file will eventually refer to macos commands once we have an implementation for them. In the meantime, and as long as react-native-macos is a fork of facebook/react-native, it is still useful to use the ios and android CLI locally for testing purposes. But when installed in another repo, such as any of the react-native-community repos along side facebook react-native, we cannot allow the CLI commands to execute twice for each installation.

In react-native.config.js, check the basename of __dirname and if it is 'react-native-macos' as it will be when installed in a 'node_modules' folder, then use empty command arrays for ios and android.

react-native-webview will be the first react-native-community repo that will need this fix in order to preserve react-native run-ios and react-native run-android functionality (see react-native-webview/react-native-webview#1164)

Microsoft Reviewers: Open in CodeFlow

@tom-un tom-un requested a review from acoates-ms as a code owner January 24, 2020 01:29
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

@pull-bot
Copy link

pull-bot commented Jan 24, 2020

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.

CATEGORY may be:
  • General
  • 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.
📖 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

Generated by 🚫 dangerJS against 6124a50

@tom-un tom-un requested a review from HeyImChris January 24, 2020 01:32
@ghost ghost removed the Needs: Author Feedback label Jan 24, 2020
Copy link
Collaborator

@acoates-ms acoates-ms left a comment

Choose a reason for hiding this comment

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

Shouldn't it be:

const ios = require('@react-native-community/cli-platform-ios');
const android = require('@react-native-community/cli-platform-android');

// Remove commands so that react-native-macos can coexist with react-native in repos that depend on both.
const path = require("path");
const isReactNativeMacOS = path.basename(__dirname) === 'react-native-macos'; 

 module.exports = isReactNativeMacOS ? {} :  {
  commands: [...ios.commands, ...android.commands],
  platforms: {
    ios: {
      linkConfig: ios.linkConfig,
      projectConfig: ios.projectConfig,
      dependencyConfig: ios.dependencyConfig,
    },
    android: {
      linkConfig: android.linkConfig,
      projectConfig: android.projectConfig,
      dependencyConfig: android.dependencyConfig,
    },
  },
  /**
   * Used when running RNTester (with React Native from source)
   */
  reactNativePath: '.',
  project: {
    ios: {
      project: './RNTester/RNTester.xcodeproj',
    },
    android: {
      sourceDir: './RNTester',
    },
  },
};

@tom-un
Copy link
Collaborator Author

tom-un commented Jan 24, 2020

Shouldn't it be:

const ios = require('@react-native-community/cli-platform-ios');
const android = require('@react-native-community/cli-platform-android');

// Remove commands so that react-native-macos can coexist with react-native in repos that depend on both.
const path = require("path");
const isReactNativeMacOS = path.basename(__dirname) === 'react-native-macos'; 

 module.exports = isReactNativeMacOS ? {} :  {
  commands: [...ios.commands, ...android.commands],
  platforms: {
    ios: {
      linkConfig: ios.linkConfig,
      projectConfig: ios.projectConfig,
      dependencyConfig: ios.dependencyConfig,
    },
    android: {
      linkConfig: android.linkConfig,
      projectConfig: android.projectConfig,
      dependencyConfig: android.dependencyConfig,
    },
  },
  /**
   * Used when running RNTester (with React Native from source)
   */
  reactNativePath: '.',
  project: {
    ios: {
      project: './RNTester/RNTester.xcodeproj',
    },
    android: {
      sourceDir: './RNTester',
    },
  },
};

Ya, that makes more sense. When we eventually have mac CLI it will have to be a non-empty object but will have nothing shared with ios or android.

@tom-un
Copy link
Collaborator Author

tom-un commented Jan 24, 2020

Shouldn't it be:

const ios = require('@react-native-community/cli-platform-ios');
const android = require('@react-native-community/cli-platform-android');

// Remove commands so that react-native-macos can coexist with react-native in repos that depend on both.
const path = require("path");
const isReactNativeMacOS = path.basename(__dirname) === 'react-native-macos'; 

 module.exports = isReactNativeMacOS ? {} :  {
  commands: [...ios.commands, ...android.commands],
  platforms: {
    ios: {
      linkConfig: ios.linkConfig,
      projectConfig: ios.projectConfig,
      dependencyConfig: ios.dependencyConfig,
    },
    android: {
      linkConfig: android.linkConfig,
      projectConfig: android.projectConfig,
      dependencyConfig: android.dependencyConfig,
    },
  },
  /**
   * Used when running RNTester (with React Native from source)
   */
  reactNativePath: '.',
  project: {
    ios: {
      project: './RNTester/RNTester.xcodeproj',
    },
    android: {
      sourceDir: './RNTester',
    },
  },
};

Ya, that makes more sense. When we eventually have mac CLI it will have to be a non-empty object but will have nothing shared with ios or android.

Actually, it doesn't work. If I have an empty exports for mac, then metro fails to bundle for mac. Its probably pointing out some other problem. The min that has to be present is

  platforms: {
    ios: {},
  },

@acoates-ms
Copy link
Collaborator

Doesn't totally surprise me. I suspect you are piggy backing on the ios stuff more than we should long term.

@tom-un
Copy link
Collaborator Author

tom-un commented Jan 24, 2020

Yes. For now I want to go with this fix as-is and detangle the iOS dependency separately.


module.exports = {
commands: [...ios.commands, ...android.commands],
commands: [...iosCommands, ...androidCommands],

Choose a reason for hiding this comment

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

Why don't we need a ...macCommands here too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will, but mac commands don't exist yet. We have to create a @react-native-community/cli-platform-macos package first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, the commands can be in this repo. No need to make that a separate repo. Windows implements its commands within react-native-windows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only reason the android and ios ones are separate is so that the community can handle the PRs etc without waiting on facebook, since facebook doesn't use the cli.

@tom-un tom-un merged commit 96ffd35 into microsoft:master Jan 24, 2020
@tom-un tom-un deleted the tomun/cli branch May 8, 2020 21:49
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.

5 participants