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

Correct RCTAnimation import #18050

Closed
wants to merge 1 commit into from

Conversation

LukeDurrant
Copy link
Contributor

Motivation

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

Test Plan

My app wouldn't build without changing this line

Related PRs

PR #16192
PR #16271
PR #17764

Release Notes

[IOS] [BREAKING] [PODS] - Fixed RCTAnimation import for integrating with cocoapods

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

Copy link
Contributor

@janicduplessis janicduplessis left a comment

Choose a reason for hiding this comment

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

Sadly this breaks normal xcode build (you can see the error on CI). Maybe it would work if we change the copy headers phase of RCTAnimation.xcodeproj to copy to the React/ folder instead of RCTAnimation/

@LukeDurrant
Copy link
Contributor Author

LukeDurrant commented Feb 26, 2018

Appears the RCTAnimation.xcodeproj doesn't build before this PR as well..
Looked further into how the CI tests are running and theres a RNTester project that you have to open

@LukeDurrant
Copy link
Contributor Author

I think maybe a better solution is to move the correct files to make the correct files public in the Header Phase. See if this fixes the tests and allows cocoa pods integration

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. cla signed labels Feb 26, 2018
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@sdwilsh sdwilsh removed the cla signed label Mar 1, 2018
@janicduplessis
Copy link
Contributor

@LukeDurrant Were you able to get it to build locally without cocoapods? Looks like CI is still failing :(

I do like this approach though.

@LukeDurrant
Copy link
Contributor Author

No can't get it to work in both

@msand
Copy link
Contributor

msand commented Mar 10, 2018

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

@hramos hramos added the Platform: iOS iOS applications. label Mar 13, 2018
@facebook-github-bot
Copy link
Contributor

@LukeDurrant 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.

@LukeDurrant LukeDurrant force-pushed the task/animate-header branch 2 times, most recently from 82b84cd to 3b7a15d Compare April 11, 2018 13:35
@LukeDurrant LukeDurrant force-pushed the task/animate-header branch 2 times, most recently from 3f8a82d to e66d28d Compare April 11, 2018 13:57
@LukeDurrant
Copy link
Contributor Author

Sorry looks like a few people got notified for review, rebased to the wrong branch and made lots of proposed file changes

@LukeDurrant
Copy link
Contributor Author

Appears ebd12fa has broken all the tests... unsure why this was even merged..

@facebook-github-bot
Copy link
Contributor

@LukeDurrant 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.

@LukeDurrant LukeDurrant force-pushed the task/animate-header branch from e66d28d to 73a8aa2 Compare May 13, 2018 03:14
@hramos
Copy link
Contributor

hramos commented May 14, 2018

Does the PR description need to be updated?

@LukeDurrant
Copy link
Contributor Author

No different solution to the same problem react native doesn’t build when integrating using cocopods

vovkasm added a commit to vovkasm/react-native that referenced this pull request Jun 5, 2018
@facebook-github-bot
Copy link
Contributor

@LukeDurrant 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.

@vovkasm
Copy link
Contributor

vovkasm commented Jun 18, 2018

I think current diff should work with CocoaPods and Xcode integration variants. Personally I tested this changeset with CocoaPods variant and it works.

Can I help further to make this change landed?

@soranoba
Copy link

What is the status of this PR?
I believe that this fix is correct, and I'm hoping for an early solution.

I think that import seems to be successful from the error of CI.
Will not it be resolved by rebase or retest?

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 9, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@ikesyo
Copy link
Contributor

ikesyo commented Sep 6, 2018

@hramos Could you please retry to import this?

hramos pushed a commit that referenced this pull request Sep 11, 2018
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
@LukeDurrant LukeDurrant deleted the task/animate-header branch September 12, 2018 00:32
gengjiawen pushed a commit to gengjiawen/react-native that referenced this pull request Sep 14, 2018
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
aleclarson pushed a commit to aleclarson/react-native that referenced this pull request Sep 16, 2018
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
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
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
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
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
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. Import Started This pull request has been imported. This does not imply the PR has been approved. Needs: Imported Diff Waiting on Meta Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants