Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link all platforms #17834

Closed
wants to merge 2 commits into from
Closed

Link all platforms #17834

wants to merge 2 commits into from

Conversation

rozele
Copy link
Contributor

@rozele rozele commented Feb 2, 2018

Motivation

@grabbou requested that we eliminate the special cases for iOS and Android in link and unlink commands.

Test Plan

  • Ensure jest tests continue to work.
  • Link and unlink a module for an iOS project without pods
  • Link and unlink a module for an iOS project with pods
  • Link and unlink a module for Android
  • Link and unlink assets on iOS and Android

Related PRs

#17745

Release Notes

[INTERNAL] [ENHANCEMENT ] [local-cli/info/link.js] - Make link and unlink more maintainable for all platforms.

This change adds hooks via the `package.json` for a platform plugin to specify hooks for generating platform configurations. This change also adds hooks to allow platform plugins to participate in `link` and `unlink` commands. The goal is to move platform-specific code for platform plugins into their own repositories / node_modules.
@rozele rozele requested review from grabbou and Kureev as code owners February 2, 2018 12:29
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cla signed labels Feb 2, 2018
@pull-bot
Copy link

pull-bot commented Feb 2, 2018

Warnings
⚠️

📋 Release Notes - This PR may have incorrectly formatted Release Notes.

⚠️

❗ Big PR - This PR is extremely unlikely to get reviewed because it touches 944 lines.

@facebook-github-bot label Needs more information

@facebook-github-bot large-pr

Attention: @grabbou, @Kureev

Generated by 🚫 dangerJS

@@ -126,3 +126,5 @@ exports.dependencyConfig = function dependencyConfigAndroid(folder, userConfig)

return { sourceDir, folder, manifest, packageImportPath, packageInstance };
};

exports.linkConfig = require('../../link/android');
Copy link
Contributor Author

@rozele rozele Feb 2, 2018

Choose a reason for hiding this comment

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

Add same for iOS #Closed

};
const linkConfig = platforms[platform] && platforms[platform].linkConfig && platforms[platform].linkConfig();
if (!linkConfig || !linkConfig.isInstalled || !linkConfig.unregister) {
return null;
Copy link
Contributor Author

@rozele rozele Feb 2, 2018

Choose a reason for hiding this comment

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

null [](start = 15, length = 4)

return; #Closed

const isIosInstalled = isInstalledIOS(projectConfig, dependencyConfig);
const isPodInstalled = isInstalledPods(projectConfig, dependencyConfig);
if (isIosInstalled) {
const allDependencies = getDependencyConfig(config, getProjectDependencies());
Copy link
Contributor Author

@rozele rozele Feb 2, 2018

Choose a reason for hiding this comment

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

getProjectDependencies [](start = 56, length = 22)

need to import #Closed

const isPodInstalled = isInstalledPods(projectConfig, dependencyConfig);
if (isIosInstalled) {
const allDependencies = getDependencyConfig(config, getProjectDependencies());
const otherDependencies = filter(allDependencies, d => d.name !== packageName);
Copy link
Contributor Author

@rozele rozele Feb 2, 2018

Choose a reason for hiding this comment

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

packageName [](start = 70, length = 11)

name? #Closed

registerDependencyPods(name, dependencyConfig, projectConfig);
}
else {
registerDependencyIOS(name, dependencyConfig, projectConfig);
Copy link
Contributor Author

@rozele rozele Feb 2, 2018

Choose a reason for hiding this comment

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

name [](start = 26, length = 4)

I don't think this takes a name param. #Closed

The previous commit added logic to allow platforms to self-configure their link behavior. The commit kept a special case for iOS and Android intact. This commit removes the special cases.
@rozele rozele closed this Feb 2, 2018
@rozele rozele deleted the linkAllPlatforms branch February 2, 2018 13:55
@grabbou
Copy link
Contributor

grabbou commented Feb 5, 2018

@rozele do you think we should cherry-pick this back to 0.53/0.52/0.51?

@rozele
Copy link
Contributor Author

rozele commented Feb 6, 2018

@grabbou - yes please do cherry-pick back to these versions (and 0.50 if possible), but this PR was closed in favor of #17745.

@grabbou
Copy link
Contributor

grabbou commented Feb 8, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants