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

add podspec for autolinking support in RN 0.6x #358

Merged
merged 7 commits into from
Dec 4, 2019

Conversation

snobear
Copy link
Contributor

@snobear snobear commented Oct 3, 2019

Add podspec to support autolinking in React Native 0.6x.

Solves issue #332

@yasithdev
Copy link
Contributor

Huge +1 for this PR! Thanks @snobear, hope it gets merged soon

@snobear
Copy link
Contributor Author

snobear commented Oct 10, 2019

Eh this podspec isn't sufficient to get opentok-react-native working with RN 0.6x. With a fresh project using react-native init (RN 0.61.2), the iOS build throws:

`React/RCTBridgeModule.h` file not found

image

I've tried a thousand different things, but not exactly sure what else we need to tweak in this library to get it working with RN 0.6.

Steps to Reproduce

  1. Start a new project and add opentok-react-native:
react-native init FreshProject
cd FreshProject
yarn add "https://github.com/snobear/opentok-react-native#add-podspec"
cd ios
pod install
cd ..
  1. Open <YourProjectName>.xcworkspace contents in XCode. This file can be found in the ios folder of your React Native project. Click on your project in top left of Xcode so its highlighted.

  2. Click File and New File

  3. Add an empty swift file to your project:

    • You can name this file anything i.e: OTInstall.swift. This is done to set some flags in XCode so the Swift code can be used.
  4. Click Create Bridging Header when you're prompted with the following modal: Would you like to configure an Objective-C bridging header?

  5. Ensure you have enabled both camera and microphone usage by adding the following entries to your Info.plist file:

<key>NSCameraUsageDescription</key>
<string>Your message to user when the camera is accessed for the first time</string>
<key>NSMicrophoneUsageDescription</key>
<string>Your message to user when the microphone is accessed for the first time</string>
  1. cd to root of your project and fire it up
react-native run-ios

From here its a bunch of header file not found errors.

I've tried tweaking the opentok-react-native/ios/OpenTokReactNative.xcodeproj/project.pbxproj file to remove HEADER_SEARCH_PATHS and then cleaning the build and xcode and reinstalling pods, but no luck. It seems like the OT's pbxproj is ignored completely. I was thinking it was overriding my projects search paths, but I guess not.

I can move this to an issue instead of a PR, but wanted to get a discussion going. Let me know if anyone has any troubleshooting ideas.

@snobear
Copy link
Contributor Author

snobear commented Oct 10, 2019

OK fixed the header issues by adding the required dependencies to the podspec file:

  s.dependency 'React'
  s.dependency 'OpenTok' 

Now getting the following:

Undefined symbols for architecture x86_64:
  "_OBJC_CLASS_$_OTPublisherSwift", referenced from:
      objc-class-ref in libopentok-react-native.a(OTPublisher.o)
      l_OBJC_$_CATEGORY_OTPublisherSwift_$_RCTExternModule in libopentok-react-nativ
  "_OBJC_CLASS_$_OTSessionManager", referenced from:
      objc-class-ref in libopentok-react-native.a(OTSessionManager.o)
      l_OBJC_$_CATEGORY_OTSessionManager_$_RCTExternModule in libopentok-react-nativ
  "_OBJC_CLASS_$_OTSubscriberSwift", referenced from:
      objc-class-ref in libopentok-react-native.a(OTSubscriber.o)
      l_OBJC_$_CATEGORY_OTSubscriberSwift_$_RCTExternModule in libopentok-react-nati
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Gettin there! Tried adding JavascriptCore.Framework` under the target's Build Phases > Link Binary With Libraries per other suggestions but no luck on that.

@yasithdev
Copy link
Contributor

Hello @snobear,

I suspect it is because you are only adding the .h and .m files in the src_files import. Maybe you should add the .swift files there too?

@yasithdev
Copy link
Contributor

yasithdev commented Oct 11, 2019

Hello, @snobear good news, I confirmed that using

s.source_files  = "ios/**/*.{h,m,swift}"

resolves the issue you mentioned! However, this seems to lead to a new problem, where the two interfaces:

@interface OTPublisher
@interface OTSubscriber

are defined twice: One in the OpenTok.framework, and one in the opentok-react-native library. One of these headers should be made private. @snobear or anyone, any ideas?

@yasithdev
Copy link
Contributor

Hello @snobear, I've created Issue #361 to handle the duplicate header issue. If this gets merged into master, then I bet this PR will work as expected after you set

s.source_files  = "ios/**/*.{h,m,swift}"

@snobear
Copy link
Contributor Author

snobear commented Oct 11, 2019

Nice, thanks for working through this with me @yasithmilinda. Adding swift then rerunning cd ios && pod install fixed the symbol issues.

I get the duplicate header issue now as you mentioned...I'll try the changes in your PR and then do some further testing of actual RN code to make sure all is looking good.

@th317erd
Copy link

Hey, can we please get this merged?

@snobear
Copy link
Contributor Author

snobear commented Oct 11, 2019

All looks good on iOS after merging #361 to fix the duplicate headers.

However, this has not been tested on RN <0.60, which @yasithmilinda will do to make sure those duplicate header removals don’t have a negative effect on older versions.

@yasithdev
Copy link
Contributor

@th317erd @snobear I've just confirmed that the changes in header files in #361 do not break existing functionality in RN 0.58 and 0.59.

@snobear
Copy link
Contributor Author

snobear commented Oct 17, 2019

@yasithmilinda awesome! This PR should be good to merge into master then IMO.

@th317erd
Copy link

Awesome! Thank you @yasithmilinda! :)

@yasithdev
Copy link
Contributor

yasithdev commented Oct 19, 2019

Awesome! Thank you @yasithmilinda! :)

you're welcome @th317erd!

@yasithdev
Copy link
Contributor

Hello folks, can we get this feature merged?

@enricop89
Copy link
Contributor

Hi guys, thank you for the great job here.
I hope to have time next week to review it. I'm sorry for the delay.

@barunprasad
Copy link

Thank you for the work so far. Are there any updates on this?

@mhdtouban
Copy link

When are you going to release it?

@barunprasad
Copy link

Need this feature soon as we are not able to test this library on a native device without this feature. Greatly appreciate if the PR is merged.

@ggoldens
Copy link
Contributor

Hi @barunprasad, sorry the delay on this. We're currently reviewing this PR.
Just by curiosity, why are you not able to test this library without this PR? If I understand it well, it's just an enhancement, not a bug fix.

@ggoldens
Copy link
Contributor

@snobear maybe I'm missing a step but I'm getting this after following your steps with RN 0.61 on a fresh project.

Screen Shot 2019-11-12 at 3 55 24 PM

@barunprasad
Copy link

@ggoldens thank you for the response. We are currently on the latest version of RN 0.61.x and we constantly upgrade to the latest version which needs all libraries to move to auto linking (not use react-native link). All our dependencies have auto-linking support. You can say that for this library we can make an exception and do the manual setup. But we wanted to make sure we only add those libraries to our project which does not block us from upgrading our projects in the future. Hence the request to have this feature ready.
Anyhow, you guys have been doing great work on this. Hope to see this feature available soon.

@th317erd
Copy link

@barunprasad For now you could always do what I am doing for now: Create a postinstall script in your package.json that creates the podspec file. I created a script that scans all react-native modules, and creates a podspec for any missing podspecs. This has enabled us on our team to autolink any library.

@ggoldens
Copy link
Contributor

@snobear were you able to reproduce the error I put above? thanks.

@snobear
Copy link
Contributor Author

snobear commented Nov 13, 2019

@ggoldens sorry been tied up. Taking a look now.

@snobear
Copy link
Contributor Author

snobear commented Nov 13, 2019

FYI I_was getting a new error:

[!] Unable to determine Swift version for the following pods:

- `opentok-react-native` does not specify a Swift version and none of the targets (`NewProject`) integrating it have the `SWIFT_VERSION` attribute set. Please contact the author or set the `SWIFT_VERSION` attribute in at least one of the targets that integrate this pod.

but fixed by specifying s.swift_version in the podspec (added to this PR's branch).

@snobear
Copy link
Contributor Author

snobear commented Nov 13, 2019

@ggoldens I'm able to install the PR with a fresh 0.61.4 RN project. You definitely shouldn't get complaints about React being available, as the podspec has it specified as a dependency. I would double-check that node_modules/opentok-react-native/opentok-react-native.podspec is present and has the dependencies listed, i.e. the podspec is the same version as in this PR. There were a few updates made along the way in this discussion, so make sure you have the latest.

Also try clearing out cache as specified in the simulator, and close any running metro bundler and Xcode instances if they're running before doing a new project.

Can some other folks please try installing this w/ a new project per install steps?

My system info in case its helpful:

$ react-native info
info Fetching system and libraries information...
System:
    OS: macOS Mojave 10.14.4
    CPU: (8) x64 Intel(R) Core(TM) i7-3840QM CPU @ 2.80GHz
    Memory: 1.05 GB / 16.00 GB
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 12.1.0 - /usr/local/bin/node
    Yarn: 1.16.0 - /usr/local/bin/yarn
    npm: 6.12.0 - /usr/local/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  SDKs:
    iOS SDK:
      Platforms: iOS 13.0, DriverKit 19.0, macOS 10.15, tvOS 13.0, watchOS 6.0
  IDEs:
    Xcode: 11.0/11A420a - /usr/bin/xcodebuild
  npmPackages:
    react: 16.9.0 => 16.9.0 
    react-native: 0.61.4 => 0.61.4 
  npmGlobalPackages:
    react-native-cli: 2.0.1
    react-native-git-upgrade: 0.2.7

@mhdtouban
Copy link

I was able to integrate @snobear PR into our project and it's working fine

@snobear snobear mentioned this pull request Nov 23, 2019
2 tasks
@snobear
Copy link
Contributor Author

snobear commented Nov 23, 2019

Hi all, just checking if we can get two other reviewers to review/merge please? cc @ggoldens @enricop89

@ggoldens
Copy link
Contributor

ggoldens commented Dec 4, 2019

@snobear, can you point this PR to the branch 0.12.2 please?

@snobear snobear changed the base branch from master to 0.12.2 December 4, 2019 18:03
@snobear
Copy link
Contributor Author

snobear commented Dec 4, 2019

@ggoldens done.

Copy link
Contributor

@ggoldens ggoldens left a comment

Choose a reason for hiding this comment

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

LGTM

@ggoldens ggoldens merged commit 4b2b9ec into opentok:0.12.2 Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants