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

make React Native autolinkable #865

Closed
2 tasks
sibelius opened this issue Nov 25, 2019 · 13 comments · Fixed by #1107
Closed
2 tasks

make React Native autolinkable #865

sibelius opened this issue Nov 25, 2019 · 13 comments · Fixed by #1107
Assignees
Milestone

Comments

@sibelius
Copy link
Member

Describe the Feature

Right now we need to add a lot of pod specs to link react-native

check this https://github.com/facebook/react-native/blob/master/template/ios/Podfile#L42

pod 'FBLazyVector', :path => "../node_modules/react-native/Libraries/FBLazyVector"
  pod 'FBReactNativeSpec', :path => "../node_modules/react-native/Libraries/FBReactNativeSpec"
  pod 'RCTRequired', :path => "../node_modules/react-native/Libraries/RCTRequired"
  pod 'RCTTypeSafety', :path => "../node_modules/react-native/Libraries/TypeSafety"
  pod 'React', :path => '../node_modules/react-native/'
  pod 'React-Core', :path => '../node_modules/react-native/'
  pod 'React-CoreModules', :path => '../node_modules/react-native/React/CoreModules'
  pod 'React-Core/DevSupport', :path => '../node_modules/react-native/'
  pod 'React-RCTActionSheet', :path => '../node_modules/react-native/Libraries/ActionSheetIOS'
  pod 'React-RCTAnimation', :path => '../node_modules/react-native/Libraries/NativeAnimation'
  pod 'React-RCTBlob', :path => '../node_modules/react-native/Libraries/Blob'
  pod 'React-RCTImage', :path => '../node_modules/react-native/Libraries/Image'
  pod 'React-RCTLinking', :path => '../node_modules/react-native/Libraries/LinkingIOS'
  pod 'React-RCTNetwork', :path => '../node_modules/react-native/Libraries/Network'
  pod 'React-RCTSettings', :path => '../node_modules/react-native/Libraries/Settings'
  pod 'React-RCTText', :path => '../node_modules/react-native/Libraries/Text'
  pod 'React-RCTVibration', :path => '../node_modules/react-native/Libraries/Vibration'
  pod 'React-Core/RCTWebSocket', :path => '../node_modules/react-native/'

  pod 'React-cxxreact', :path => '../node_modules/react-native/ReactCommon/cxxreact'
  pod 'React-jsi', :path => '../node_modules/react-native/ReactCommon/jsi'
  pod 'React-jsiexecutor', :path => '../node_modules/react-native/ReactCommon/jsiexecutor'
  pod 'React-jsinspector', :path => '../node_modules/react-native/ReactCommon/jsinspector'
  pod 'ReactCommon/callinvoker', :path => "../node_modules/react-native/ReactCommon"
  pod 'ReactCommon/turbomodule/core', :path => "../node_modules/react-native/ReactCommon"
  pod 'Yoga', :path => '../node_modules/react-native/ReactCommon/yoga'

  pod 'DoubleConversion', :podspec => '../node_modules/react-native/third-party-podspecs/DoubleConversion.podspec'
  pod 'glog', :podspec => '../node_modules/react-native/third-party-podspecs/glog.podspec'
  pod 'Folly', :podspec => '../node_modules/react-native/third-party-podspecs/Folly.podspec'

This makes it hard to newcomers and also hard to support monorepo with yarn hoist, as we need to change from

../node_modules/react-native

to

../../../node_modules/react-native

Possible Implementations

  • provide a getNodeModulesPath('react-native') helper function for cocoapods

or

  • autolink react-native - have a react-native.config.js that tells how to properly link react-native itself
@RichardLindhout
Copy link

RichardLindhout commented Nov 25, 2019

I prefer the auto linking, since the react native libraries change a lot lately!

@vitalyiegorov
Copy link

  • for autolinking

@grabbou
Copy link
Member

grabbou commented Jan 13, 2020

Yes, this has been long on my plate as well. This is a great feature and I think we should ship it with the next.

I am going to write it down and see what's needed.

@grabbou grabbou self-assigned this Jan 13, 2020
@grabbou grabbou added this to the @next milestone Jan 13, 2020
@thymikee
Copy link
Member

thymikee commented Feb 19, 2020

This is already done on RN master. See: https://github.com/facebook/react-native/blob/d4d8887b5018782eeb3f26efa85125e6bbff73e4/template/ios/Podfile#L7 and facebook/react-native#28077 as a followup to make the name less confusing. No need to track it here, so closing.

@grabbou
Copy link
Member

grabbou commented Mar 19, 2020

It's not and I actually question whether it's not going to cause even more issues.

See https://github.com/facebook/react-native/blob/d4d8887b5018782eeb3f26efa85125e6bbff73e4/scripts/autolink-ios.rb#L8, it assumes a path.

Screenshot 2020-03-19 at 15 46 18

Hidden hardcoded path & optional use of ( and ) in Ruby = asking for troubles.

Screenshot 2020-03-19 at 15 47 29

something doesn't work in a monorepo. How can I fix it?

@grabbou grabbou reopened this Mar 19, 2020
@grabbou
Copy link
Member

grabbou commented Mar 19, 2020

Fortunately, it's not shipping yet so we got time to improve it -> facebook/react-native@a35efb9#diff-1c007079c085ea0a9262971dc43e6cea

My suggestion: call use_react_native! from within use_native_modules - https://github.com/react-native-community/cli/blob/master/packages/platform-ios/native_modules.rb. We already know all the paths here and can easily point to correct React Native location, making it completely unnecessary to modify any path when working in a mono-repo or just moving files around.

Without this change, users will need to explicitly pass path to use_react_native! if different than ../node_modules/react-native (which can vary, depending on the setup in a monorepo and hoisting)

@grabbou
Copy link
Member

grabbou commented Mar 19, 2020

CC: @alloy let me know what you think - this will be a breaking change as it's a non-backwards-compatible addition (requires change to Podfile after you upgrade to a newer React Native). We got time to ship it as a part of 0.63.

@grabbou grabbou pinned this issue Mar 19, 2020
@grabbou grabbou modified the milestones: @next, 5.x Mar 19, 2020
@alloy
Copy link
Member

alloy commented Mar 19, 2020

I actually would like to get rid of all the local path faffing. Users should retain control over what pieces of RN they actually need (i.e. it should be in their Podfile) and I’m afraid that adding more unique (to RN) abstractions is only going to make it harder for people to know how to control what they need.

@alloy
Copy link
Member

alloy commented Mar 19, 2020

Forgot to include some details; I’m thinking of a CocoaPods plugin that is able to treat node_modules as a spec-repo. This also fixes podspecs in node_modules having transitive dependencies on podspecs in other npm packages.

@alloy alloy self-assigned this Mar 19, 2020
@grabbou
Copy link
Member

grabbou commented Mar 19, 2020

Forgot to include some details; I’m thinking of a CocoaPods plugin that is able to treat node_modules as a spec-repo. This also fixes podspecs in node_modules having transitive dependencies on podspecs in other npm packages.

Sure, do you have any ETA or write-up on how you'd like to move on from this? This sounds interesting, but I am not familiar with how CocoaPods work and whether this would be easy to implement with our current behavior in mind. We probably don't want to introduce a lot of breaking changes and at the same time, realistically speaking, we haven't fully deprecated and removed legacy link code yet (a lot of work needed to do proper house keeping). Long story short, I'd like to help with this!

As much as I like the aforementioned improvement, I would treat it as a parallel track.

I was thinking to just move the use_react_native! from template to native_modules.rb and explicitly pass the path that we have already resolved and it's proven to work for past few releases.

Example (after line 34):

project_root = Pathname.new(config["project"]["ios"]["sourceDir"])

use_react_native(config["reactNativePath"])

Otherwise, it's going to ship with 0.63 and degrade monorepo support that we're working to improve.

@alloy
Copy link
Member

alloy commented Mar 19, 2020

I’m thinking of a CocoaPods plugin that is able to treat node_modules as a spec-repo.
Sure, do you have any ETA or write-up on how you'd like to move on from this?

Nothing yet. It’s probably a month or so out on my roadmap.

Long story short, I'd like to help with this!

Ace, I’ll definitely loop you in closer to that time 👌

As much as I like the aforementioned improvement, I would treat it as a parallel track.

Sure, good call 👍

I was thinking to just move the use_react_native! from template to native_modules.rb and explicitly pass the path that we have already resolved and it's proven to work for past few releases.

My concern still stands, though; moving the list of RN deps out of the user’s Podfile makes it hard to control what deps you really need. This is also why in the end I opted to not make make use of this abstraction in v0.62.

How would you solve that and does that solution outweigh leaving the noisy but explicit paths in the Podfile for a bit longer?

@grabbou
Copy link
Member

grabbou commented Mar 20, 2020

My concern still stands, though; moving the list of RN deps out of the user’s Podfile makes it hard to control what deps you really need. This is also why in the end I opted to not make make use of this abstraction in v0.62.

I see. In this case, it's probably worth revisiting - my understanding was that this has been already decided and agreed to be the best way to go forward.

How would you solve that and does that solution outweigh leaving the noisy but explicit paths in the Podfile for a bit longer?

TBH, I don't have anything against RN dependencies being explicitly declared in a Podfile. Depending on whether the aforementioned abstraction ends up being included in 0.63 or not, we may or may not advocate for moving it under use_native_modules.

As far as the path itself, the use_native_modules can always return a config and we can use that value to replace the explicit path with the implicit one. In this case, the change is completely non-breaking and transparent.

@alloy
Copy link
Member

alloy commented Mar 20, 2020

As far as the path itself, the use_native_modules can always return a config and we can use that value to replace the explicit path with the implicit one. In this case, the change is completely non-breaking and transparent.

I like that 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants