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

Fix reference to fishhook.h in RCTReconnectingWebSocket.m #16130

Closed
wants to merge 2 commits into from

Conversation

mobinni
Copy link

@mobinni mobinni commented Sep 28, 2017

Motivation

When upgrading to react-native 0.49 you get an error due to a reference to fishhook.h in RCTReconnectingWebSocket.m. When I read the fishhook docs it specified that when the fishhook.a binary is linked references should be imported as:

#import "fishhook.h"

Link to issue: #16039

https://github.com/facebook/react-native/blob/master/Libraries/fishhook/README.md

Test Plan

Upgrade from a clean react-native 0.48 to 0.49, upgrade your podspec to include fishhook.
Run the project and it should fail on this import.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Sep 28, 2017
@ide
Copy link
Contributor

ide commented Oct 4, 2017

Another commit that fixes this via the podspec has landed (#16192) and will be going out with 0.49.1 in an hour or so if the build pipeline succeeds. I'm going to close this out since that commit should fix your issue and please tell us if it doesn't.

@ide ide closed this Oct 4, 2017
@VinceBurn
Copy link
Contributor

Hi @ide I'm using react-native 0.49.3 and this issue is still valid, my project is not compiling with Xcode 9 if I don't change the source in my Pod version of 'RCTReconnectingWebSocket.m'.
(I'm on a new project, fresh install of the Pod to integrate React Native to an existing project)

This merge request need to be reopen since the problem is clearly not fixe.

@ide
Copy link
Contributor

ide commented Oct 19, 2017

OK, it's clear this is still an issue but I don't think it is right to modify the source code though I could be wrong about that, I don't have visibility into Facebook's internal build systems. The right fix I think is to figure out how to get Xcode to treat fishhook as a header directory.

@oskarsliukis
Copy link

Any updates? Can't upgrade to latest RN version due to this and changing import manually is not an option.

@@ -11,7 +11,7 @@

#import <React/RCTConvert.h>
#import <React/RCTDefines.h>
#import <fishhook/fishhook.h>
#import "fishhook.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change is correct. The fishhook.h file is not in the same directory, so importing it needs to be done through the fishhook library, e.g. #import <fishhook/fishhook.h>.

It might be the case that it's actually included under React, instead (see #16192 (comment)).

@msand
Copy link
Contributor

msand commented Mar 10, 2018

This should probably be solved using this approach: #13198 (comment)

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants