[IOS] [BUGFIX] [Fishhook] - Correct fishhook import in RCTReconnectingWebSocket Fixes #16039 #16271

macdoum1 · 2017-10-10T13:57:33Z

RCTReconnectingWebSocket is not compiling correctly because of an incorrect import (#16039).

Test Plan:

Everything build and run as usual.

Release Notes:

[IOS] [BUGFIX] [Fishhook] - Correct fishhook import in RCTReconnectingWebSocket Fixes #16039

yvbeek · 2017-10-18T01:08:52Z

Please merge this one in, it is causing a lot of confusion with CocoaPods users.

bithavoc · 2017-10-19T04:09:47Z

please merge

ide · 2017-10-19T07:38:56Z

I suspect this isn't the desired fix since this hasn't been an issue within FB's codebase or else they would have fixed the compiler error. The Xcode project needs to be set up to treat fishhook as a header directory.

DylanNWatt · 2017-10-19T14:08:43Z

What's the nuance between this and #import <React/fishhook.h>

macdoum1 · 2017-10-19T15:42:41Z

I've moved on from our implementation of react-native for now, but this likely would only be an issue for the cocoapod installation method. A #import <React/fishhook.h> is a module import, whereas a #import "fishhook.h is a file import.
Either one would work I believe, just not #import <fishhook/fishhook.h> because Cocoapod subspecs are not built as modules.

macdoum1 · 2017-10-19T15:44:05Z

@ide FB's codebase might not use react-native as a cocoapod so they wouldn't see this issue. Not sure.

bithavoc · 2017-10-19T16:08:28Z

@ide header_path doesn't work, there is a PR for it #16192

facebook-github-bot · 2017-11-09T18:04:02Z

@macdoum1 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

ide · 2017-11-09T19:43:06Z

FB's codebase might not use react-native as a cocoapod so they wouldn't see this issue. Not sure.

Yes, and their builds haven't been failing because of this so I suspect the issue is with how the pods are configured. To be on the same page I would like us to figure out how to fix compilation issues (fwiw I've tested with pods and don't run into this) without modifying the import to not impact FB's build.

macdoum1 · 2017-11-09T20:57:07Z

@ide The issue here is that cocoapod subspecs are not built as separate modules so the existing import will never work with cocoapods. From the commenters above and on the related issue it appears this happens with new cocoapod installations, so i'd imagine existing installations would work purely from some leftover DerivedData magic.

It appears none of the travis jobs/tests cover a cocoapod installation which is why this issue has probably gone undetected.

@jgsamudio mentioned in the issue thread that removing !use_frameworks resolved this issue for anyone following along: #16039 (comment) . However, if your project has any swift cocoapods this is not an option.

pull-bot · 2017-11-09T21:01:00Z

	Warnings
⚠️	📋 Test Plan - This PR appears to be missing a Test Plan.
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

tirrorex · 2017-11-10T10:14:59Z

@macdoum1 the real issue here is people working in objective-c not taking into account the swift ecosystem, we see that with google projects everyday already

facebook-github-bot · 2017-12-10T18:04:09Z

@macdoum1 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

stale · 2018-02-08T18:47:18Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

makovkastar · 2018-02-09T13:09:13Z

The issue is still there in 0.51.0.

barbieri · 2018-02-27T14:38:13Z

still happening with 0.53.3

msand · 2018-03-09T23:56:31Z

And in 0.54.0

msand · 2018-03-10T00:02:09Z

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

justColbs · 2018-03-12T17:29:08Z

This works for me, but now I can't import React without getting the "Could not build Objective-C module React" error. Any ideas? Is this related?

v 0.54.1

cobarx · 2018-04-04T23:17:43Z

I'm also seeing the issue that @justColbs described. Any idea how to work around or fix this?

v0.54.4

Edit: added version

ikesyo · 2018-06-28T15:11:17Z

That should brake non-CocoaPods installation so is not a correct fix. The technique described in #13198 (comment) would be the right direction:

#if __has_include(<React/fishhook.h>)
#import <React/fishhook.h>
#else
#import <fishhook/fishhook.h>
#endif

macdoum1 · 2018-06-28T15:15:20Z

@ikesyo Good point, making that change now

macdoum1 · 2018-06-28T15:16:43Z

@ikesyo Done!

ikesyo · 2018-06-28T15:17:04Z

You should also rebase onto current master to make CI run correctly.

macdoum1 · 2018-06-28T15:19:55Z

@ikesyo Rebased this morning, should be good to go

ikesyo · 2018-06-28T15:35:56Z

@macdoum1 It seems that the parent commit of this PR is 7809381 on 10 Nov 2017, not the today's HEAD of master. Maybe you rebased onto master of your fork, not onto master of the upstream.

macdoum1 · 2018-06-28T15:40:56Z

Alright, now we should be up to date

facebook-github-bot

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yagudaev · 2018-06-30T20:25:57Z

Still an issue in 0.55.4, I guess this will be part of 0.57? or in 0.56?

hramos · 2018-07-02T15:33:39Z

0.55.4 was released several months ago - as you can see above, the fix was merged to master 4 days ago. You can expect this in a future release - 0.57 most likely (0.56 was released last month).

ikesyo · 2018-07-03T00:43:42Z

Is the cherry-pick request to 0.56 rejected?

react-native-community/releases#14 (comment)

hramos · 2018-07-03T14:10:13Z

@ikesyo best to keep that discussion in the release thread.

…16271) Summary: RCTReconnectingWebSocket is not compiling correctly because of an incorrect import (#16039). Everything build and run as usual. [IOS] [BUGFIX] [Fishhook] - Correct fishhook import in RCTReconnectingWebSocket Fixes #16039 Closes #16271 Differential Revision: D8679758 Pulled By: hramos fbshipit-source-id: b05dda3a01a68ace87f11889b84ce6b323e5c16a

Summary: Fixing error node_modules/react-native/Libraries/NativeAnimation/RCTNativeAnimatedNodesManager.h:12:9: 'RCTAnimation/RCTValueAnimatedNode.h' file not found I'm integrating react native into an existing app through cocoa pods and similar to other PRs PR #16192 PR #16271 PR #17764 When integrating with cocoa pods ``` pod 'React', :path => './node_modules/react-native', :subspecs => [ 'Core', 'DevSupport', # Include this to enable In-App Devmenu if RN >= 0.43 'RCTText', 'RCTNetwork', 'RCTWebSocket', # needed for debugging 'RCTImage', 'RCTWebSocket', 'BatchedBridge', 'RCTLinkingIOS', 'RCTActionSheet', 'RCTAnimation' ] ``` With `RCTAnimation` being the important part for this PR. Also note without this PR and the other PR's above if anyone is trying to integrate react native into an existing app i.e. through cocoa pods they won't be able too. I think the latest working version is My app wouldn't build without changing this line PR #16192 PR #16271 PR #17764 [IOS] [BREAKING] [PODS] - Fixed RCTAnimation import for integrating with cocoapods Pull Request resolved: #18050 Differential Revision: D9235162 Pulled By: hramos fbshipit-source-id: 426daccf8d8952658e262d5a0e4623c72c38542c

Summary: Fixing error node_modules/react-native/Libraries/NativeAnimation/RCTNativeAnimatedNodesManager.h:12:9: 'RCTAnimation/RCTValueAnimatedNode.h' file not found I'm integrating react native into an existing app through cocoa pods and similar to other PRs PR facebook#16192 PR facebook#16271 PR facebook#17764 When integrating with cocoa pods ``` pod 'React', :path => './node_modules/react-native', :subspecs => [ 'Core', 'DevSupport', # Include this to enable In-App Devmenu if RN >= 0.43 'RCTText', 'RCTNetwork', 'RCTWebSocket', # needed for debugging 'RCTImage', 'RCTWebSocket', 'BatchedBridge', 'RCTLinkingIOS', 'RCTActionSheet', 'RCTAnimation' ] ``` With `RCTAnimation` being the important part for this PR. Also note without this PR and the other PR's above if anyone is trying to integrate react native into an existing app i.e. through cocoa pods they won't be able too. I think the latest working version is My app wouldn't build without changing this line PR facebook#16192 PR facebook#16271 PR facebook#17764 [IOS] [BREAKING] [PODS] - Fixed RCTAnimation import for integrating with cocoapods Pull Request resolved: facebook#18050 Differential Revision: D9235162 Pulled By: hramos fbshipit-source-id: 426daccf8d8952658e262d5a0e4623c72c38542c

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 10, 2017

johannth mentioned this pull request Oct 24, 2017

[iOS][cocoapods] 'RCTAnimation/RCTValueAnimatedNode.h' file not found #13198

Closed

chirag04 mentioned this pull request Dec 5, 2017

Fix fishhook import #17080

Closed

stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 8, 2018

stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 9, 2018

This was referenced Feb 22, 2018

Correct RCTAnimation import #18050

Closed

Fix - Unable to build React target after a 'pod install' #17764

Closed

react-native-bot added Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Ran Commands One of our bots successfully processed a command. labels Mar 16, 2018

react-native-bot added the Missing Test Plan This PR appears to be missing a test plan. label May 15, 2018

macdoum1 force-pushed the master branch from f8cf6c2 to 553e5fe Compare June 28, 2018 15:16

macdoum1 requested review from hramos, janicduplessis, mhorowitz and shergin as code owners June 28, 2018 15:39

[iOS] Correct fishhook import Fixes facebook#16039

0645a93

macdoum1 force-pushed the master branch from 9aeba7b to 0645a93 Compare June 28, 2018 15:40

macdoum1 changed the title ~~[iOS] Correct fishhook import Fixes #16039~~ [IOS] [BUGFIX] [Fishhook] - Correct fishhook import in RCTReconnectingWebSocket Fixes #16039 Jun 28, 2018

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jun 28, 2018

facebook-github-bot approved these changes Jun 28, 2018

View reviewed changes

facebook-github-bot closed this in 75a0273 Jun 28, 2018

react-native-bot added the Bug label May 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IOS] [BUGFIX] [Fishhook] - Correct fishhook import in RCTReconnectingWebSocket Fixes #16039 #16271

[IOS] [BUGFIX] [Fishhook] - Correct fishhook import in RCTReconnectingWebSocket Fixes #16039 #16271

macdoum1 commented Oct 10, 2017 •

edited

Loading

yvbeek commented Oct 18, 2017

bithavoc commented Oct 19, 2017

ide commented Oct 19, 2017

DylanNWatt commented Oct 19, 2017

macdoum1 commented Oct 19, 2017

macdoum1 commented Oct 19, 2017

bithavoc commented Oct 19, 2017

facebook-github-bot commented Nov 9, 2017

ide commented Nov 9, 2017

macdoum1 commented Nov 9, 2017 •

edited

Loading

pull-bot commented Nov 9, 2017

tirrorex commented Nov 10, 2017

facebook-github-bot commented Dec 10, 2017

stale bot commented Feb 8, 2018

makovkastar commented Feb 9, 2018

barbieri commented Feb 27, 2018

msand commented Mar 9, 2018

msand commented Mar 10, 2018

justColbs commented Mar 12, 2018 •

edited

Loading

cobarx commented Apr 4, 2018 •

edited

Loading

ikesyo commented Jun 28, 2018 •

edited

Loading

macdoum1 commented Jun 28, 2018

macdoum1 commented Jun 28, 2018

ikesyo commented Jun 28, 2018

macdoum1 commented Jun 28, 2018

ikesyo commented Jun 28, 2018 •

edited

Loading

macdoum1 commented Jun 28, 2018

facebook-github-bot left a comment

yagudaev commented Jun 30, 2018

hramos commented Jul 2, 2018

ikesyo commented Jul 3, 2018

hramos commented Jul 3, 2018

[IOS] [BUGFIX] [Fishhook] - Correct fishhook import in RCTReconnectingWebSocket Fixes #16039 #16271

[IOS] [BUGFIX] [Fishhook] - Correct fishhook import in RCTReconnectingWebSocket Fixes #16039 #16271

Conversation

macdoum1 commented Oct 10, 2017 • edited Loading

Test Plan:

Release Notes:

yvbeek commented Oct 18, 2017

bithavoc commented Oct 19, 2017

ide commented Oct 19, 2017

DylanNWatt commented Oct 19, 2017

macdoum1 commented Oct 19, 2017

macdoum1 commented Oct 19, 2017

bithavoc commented Oct 19, 2017

facebook-github-bot commented Nov 9, 2017

ide commented Nov 9, 2017

macdoum1 commented Nov 9, 2017 • edited Loading

pull-bot commented Nov 9, 2017

tirrorex commented Nov 10, 2017

facebook-github-bot commented Dec 10, 2017

stale bot commented Feb 8, 2018

makovkastar commented Feb 9, 2018

barbieri commented Feb 27, 2018

msand commented Mar 9, 2018

msand commented Mar 10, 2018

justColbs commented Mar 12, 2018 • edited Loading

cobarx commented Apr 4, 2018 • edited Loading

ikesyo commented Jun 28, 2018 • edited Loading

macdoum1 commented Jun 28, 2018

macdoum1 commented Jun 28, 2018

ikesyo commented Jun 28, 2018

macdoum1 commented Jun 28, 2018

ikesyo commented Jun 28, 2018 • edited Loading

macdoum1 commented Jun 28, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

yagudaev commented Jun 30, 2018

hramos commented Jul 2, 2018

ikesyo commented Jul 3, 2018

hramos commented Jul 3, 2018

macdoum1 commented Oct 10, 2017 •

edited

Loading

macdoum1 commented Nov 9, 2017 •

edited

Loading

justColbs commented Mar 12, 2018 •

edited

Loading

cobarx commented Apr 4, 2018 •

edited

Loading

ikesyo commented Jun 28, 2018 •

edited

Loading

ikesyo commented Jun 28, 2018 •

edited

Loading