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 both iOS and tvOS libraries with react-native link (fix #13783) #17231

Closed
wants to merge 15 commits into from

Conversation

douglowder
Copy link
Contributor

Motivation

Fix issues with the react-native CLI when linking iOS and tvOS libraries to a project created with react-native init. (#13783)

Test Plan

Verified the changes against test project at https://github.com/dlowder-salesforce/react-native-link-test. Both react-native link react-native-svg and react-native unlink react-native-svg work correctly on this project. Added new unit test for the new file added to local-cli/link/ios.

Release Notes

[CLI] [BUGFIX] react-native link has been fixed to correctly link iOS and tvOS targets.
[IOS] [BUGFIX] react-native link has been fixed to correctly link iOS and tvOS targets.

@douglowder
Copy link
Contributor Author

@grabbou please review this when you get a chance.

@pull-bot
Copy link

pull-bot commented Dec 15, 2017

Warnings
⚠️

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

@facebook-github-bot label Needs more information

Attention: @shergin

Generated by 🚫 dangerJS

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 15, 2017
* Given xcodeproj it returns list of targets
*/
module.exports = function getTargets(project) {
let targets = project.getFirstProject()['firstProject']['targets'];

Choose a reason for hiding this comment

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

dot-notation: ["firstProject"] is better written in dot notation.

* Given xcodeproj it returns list of targets
*/
module.exports = function getTargets(project) {
let targets = project.getFirstProject()['firstProject']['targets'];

Choose a reason for hiding this comment

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

dot-notation: ["targets"] is better written in dot notation.

@pull-bot
Copy link

pull-bot commented Dec 27, 2017

Warnings
⚠️

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

@facebook-github-bot label Needs more information

Attention: @shergin

Generated by 🚫 dangerJS

@douglowder
Copy link
Contributor Author

@grabbou could you please review this ASAP?

@grabbou
Copy link
Contributor

grabbou commented Jan 4, 2018 via email

@douglowder
Copy link
Contributor Author

@grabbou : @matthargett says this looks good, but is asking for automated testing of react-native link and react-native unlink with these changes. I'll go ahead and start working on that.... it's up to you whether you want to go ahead and merge this, given the severity of the bug.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 29, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@hramos
Copy link
Contributor

hramos commented Jan 29, 2018

If this does not land shortly, please try re-basing. Due to a recent change to the internal repo, some PRs older than a week (as of today) may need to be rebased in order to land cleanly.

Copy link
Contributor

@hramos hramos left a comment

Choose a reason for hiding this comment

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

Please rebase and we can try re-importing again.

@douglowder
Copy link
Contributor Author

@hramos I just merged from facebook/master, hopefully it can be landed now... Thanks!!

@douglowder
Copy link
Contributor Author

@hramos after the last merge from master, I'm hitting #17742 when I test this locally... FYI

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Plo4ox pushed a commit to Plo4ox/react-native that referenced this pull request Feb 17, 2018
…#13783)

Summary:
Fix issues with the react-native CLI when linking iOS and tvOS libraries to a project created with `react-native init`. (facebook#13783)

Verified the changes against test project at https://github.com/dlowder-salesforce/react-native-link-test.  Both `react-native link react-native-svg` and `react-native unlink react-native-svg` work correctly on this project.  Added new unit test for the new file added to `local-cli/link/ios`.

[CLI] [BUGFIX] `react-native link` has been fixed to correctly link iOS and tvOS targets.
[IOS] [BUGFIX] `react-native link` has been fixed to correctly link iOS and tvOS targets.
Closes facebook#17231

Differential Revision: D6837567

Pulled By: hramos

fbshipit-source-id: 234d3d3966ae1b89cd16a37c95d303553f7ba5f5
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Mar 1, 2018
Summary:
Fix issues with the react-native CLI when linking iOS and tvOS libraries to a project created with `react-native init`. (#13783)

Verified the changes against test project at https://github.com/dlowder-salesforce/react-native-link-test.  Both `react-native link react-native-svg` and `react-native unlink react-native-svg` work correctly on this project.  Added new unit test for the new file added to `local-cli/link/ios`.

[CLI] [BUGFIX] `react-native link` has been fixed to correctly link iOS and tvOS targets.
[IOS] [BUGFIX] `react-native link` has been fixed to correctly link iOS and tvOS targets.
Closes facebook/react-native#17231

Differential Revision: D6837567

Pulled By: hramos

fbshipit-source-id: 234d3d3966ae1b89cd16a37c95d303553f7ba5f5
@douglowder douglowder deleted the tvos-cli-link-fix-2 branch April 6, 2018 18:06
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
Fix issues with the react-native CLI when linking iOS and tvOS libraries to a project created with `react-native init`. (#13783)

Verified the changes against test project at https://github.com/dlowder-salesforce/react-native-link-test.  Both `react-native link react-native-svg` and `react-native unlink react-native-svg` work correctly on this project.  Added new unit test for the new file added to `local-cli/link/ios`.

[CLI] [BUGFIX] `react-native link` has been fixed to correctly link iOS and tvOS targets.
[IOS] [BUGFIX] `react-native link` has been fixed to correctly link iOS and tvOS targets.
Closes facebook/react-native#17231

Differential Revision: D6837567

Pulled By: hramos

fbshipit-source-id: 234d3d3966ae1b89cd16a37c95d303553f7ba5f5
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. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants