mkuczera / react-native-haptic-feedback

React-Native Haptic Feedback for iOS with Android similar behaviour.
MIT License
861 stars 106 forks source link

Enable react-native-haptic-feedback to be included in Podfile #23

Closed esilverberg closed 5 years ago

esilverberg commented 5 years ago

Our project includes react native modules via pod 'react-native-haptic-feedback', :path => 'node_modules/react-native-haptic-feedback’

Because of the mismatch in names between the project and the .podspec, this was not possible

I have renamed the .podspec file and further made explicit the dependency on React

mkuczera commented 5 years ago

Hi, thank you for the PR. The React dependecy was removed in #22 due to the fact that it is not needed anymore and resolves in duplicates

esilverberg commented 5 years ago

Hmm...I seemed to require that dependency in my project or else I got file not found errors because of an out-of-order build. For example, this line here:

https://github.com/mkuczera/react-native-haptic-feedback/blob/master/ios/RNReactNativeHapticFeedback.h#L7

This expects React to have been already compiled in order to locate the header file.

mkuczera commented 5 years ago

Okay, sure. I will just check it this evening and test it out. Maybe in the meantime @jfrolich can give clarification. I´m unfortunately not a iOS expert :D

jfrolich commented 5 years ago

Hmm...I seemed to require that dependency in my project or else I got file not found errors because of an out-of-order build. For example, this line here:

https://github.com/mkuczera/react-native-haptic-feedback/blob/master/ios/RNReactNativeHapticFeedback.h#L7

This expects React to have been already compiled in order to locate the header file.

I don't think you need to compile react to get the header, but if react is compiled out of order you might have to change the build order in you scheme:

image

esilverberg commented 5 years ago

So the issue is that I am using Cocoapods to configure my workspace, and Cocoapods is the one who configures my .xcodeproj. That is the first part of this change -- renaming the files so that it is compatible with inclusion by Cocoapods. The second part -- adding the dependency -- ensures that Cocoapods does build in the proper order. I could manually adjust it as you show above, but the next time I run pod install I think it may get out of order.

Is there a downside to explicitly naming this dependency on react?

jfrolich commented 5 years ago

I also use cocoa pods for all my dependencies and it works fine. Some projects use cocoa pods, but manage the react dependency manually.

esilverberg commented 5 years ago

I have six other dependencies that are included in my Podfile, and all of them express a dependency on React:

https://github.com/react-community/lottie-react-native/blob/master/lottie-react-native.podspec#L18 https://github.com/corbt/react-native-keep-awake/blob/master/react-native-keep-awake.podspec#L21 https://github.com/inProgress-team/react-native-youtube/blob/master/react-native-youtube.podspec#L17 https://github.com/DylanVann/react-native-fast-image/blob/master/react-native-fast-image.podspec#L22 https://github.com/stefalda/ReactNativeLocalization/blob/master/ReactNativeLocalization.podspec#L18 https://github.com/react-native-community/react-native-video/blob/master/react-native-video.podspec#L32

react-native-haptic-feedback is the only project that does not express this dependency, and this omission breaks inclusion via Cocopods without a manual edit of the .xcodeproject.

Unless there is some other reason for this omission, I don't see why it would not be included in the podfile.

mkuczera commented 5 years ago

I will merge this PR soon. Also saw at different Libraries that they include this in the podspec. I would go now for the reinclude because the same issue that @esilverberg mentioned is currently occuring in our company.