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

Use yoga from cocoapods instead of local podspec #26053

Closed
wants to merge 1 commit into from

Conversation

janicduplessis
Copy link
Contributor

Summary

I think yoga is stable enough to be used from cocoapods instead of relying on the internal vendored version. This only affects install with cocoapods.

The main motivation behind this is better interop with other libraries that use yoga also from cocoapods (in my case Flipper). Right now it causes an error because of multiple copies of libyoga.a. By doing this they will share the same copy of yoga.

Changelog

[iOS] [Changed] - Use yoga from cocoapods instead of local podspec

Test Plan

Tested locally in an app and made sure it builds still.
Tested in RNTester

@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. Contributor A React Native contributor. labels Aug 13, 2019
@react-native-bot react-native-bot added the Platform: iOS iOS applications. label Aug 13, 2019
@dulmandakh
Copy link
Contributor

I find that Yoga releases lag behind the master, while React Native use Yoga from the master. I'm afraid that there might be breakages due to the mismatch. We had a case where Buck release delay broke Android CI.

@grabbou
Copy link
Contributor

grabbou commented Aug 22, 2019

Hm, @janicduplessis, does it mean we can't debug the layout with Flipper right now, without this PR?

@janicduplessis
Copy link
Contributor Author

@grabbou Yea, we can't include the layout plugin because it depends on Yoga from pods and conflicts with the one from RN.

@ajanuar
Copy link

ajanuar commented Aug 29, 2019

Waiting for this PR to be merged, I have case when we want to use YogaKit in our native page, there is conflict between Yoga in ReactCommon and Yoga as dependency of YogaKit.

@dulmandakh
Copy link
Contributor

dulmandakh commented Aug 29, 2019 via email

@axemclion
Copy link
Contributor

@janicduplessis - I spoke about this internally, and with the Flipper people. I think the biggest risk of moving Yoga out of React Native would be that we would need to ensure that Yoga and React Native are released in lock-step.

Instead of moving Yoga out, I have a PR, where i change the Yoga version in the yoga.podspec file to match that of the real yoga. This should solve the flipper issue, since both pods will point to the same version. IMHO, doing this will be a smaller change. What do you think ?

@janicduplessis
Copy link
Contributor Author

I'm not sure how cocoapods resolve dependencies but wouldn't that be dangerous since the implementation can diverge for the same version? Are we sure it would resolve to the local podspec included in RN?

I'm fine with landing some compromise if it unblocks flipper. Ideally we'd have regular yoga releases whenever important fixes or improvements are made and the version is bumped in RN at the same time, like we do with metro.

@axemclion
Copy link
Contributor

Here are the diffs I am talking about

Can you apply these changes, and see if Flipper is able to work ?

@janicduplessis
Copy link
Contributor Author

@axemclion I'm getting this error

[!] The following Swift pods cannot yet be integrated as static libraries:

The Swift pod `YogaKit` depends upon `Yoga`, which does not define modules. To opt into those targets generating module maps (which is necessary to import them from Swift when building as static libraries), you may set `use_modular_headers!` globally in your Podfile, or specify `:modular_headers => true` for particular dependencies.

Adding

spec.pod_target_xcconfig = {
      'DEFINES_MODULE' => 'YES'
  }

in the yoga podspec fixes the issue (taken this from the one in the yoga repo)

@janicduplessis
Copy link
Contributor Author

This PR is no longer needed

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. Contributor A React Native contributor. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants