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

Update RCTUIManager.m #15784

Closed
wants to merge 1 commit into from
Closed

Update RCTUIManager.m #15784

wants to merge 1 commit into from

Conversation

iegik
Copy link

@iegik iegik commented Sep 4, 2017

To make React Native play nicely with our internal build infrastructure we need to properly namespace all of our header includes.

Where previously you could do #import "RCTBridge.h", you must now write this as #import <React/RCTBridge.h>. If your xcode project still has a custom header include path, both variants will likely continue to work, but for new projects, we're defaulting the header include path to $(BUILT_PRODUCTS_DIR)/usr/local/include, where the React and CSSLayout targets will copy a subset of headers too. To make Xcode copy headers phase work properly, you may need to add React as an explicit dependency to your app's scheme and disable "parallelize build".

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

@iegik iegik requested a review from shergin as a code owner September 4, 2017 11:39
@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. GH Review: review-needed labels Sep 4, 2017
@pull-bot
Copy link

pull-bot commented Sep 4, 2017

Attention: @shergin

Generated by 🚫 dangerJS

@hramos
Copy link
Contributor

hramos commented Sep 8, 2017

Do you have a test plan?

@shergin
Copy link
Contributor

shergin commented Sep 9, 2017

This is a very radical and breaking change which conflicts with established guidelines and practices.
If you are strongly believe that we should reconsider it, please open an issue and provide detailed motivation and strong argumentation there.

@shergin shergin closed this Sep 9, 2017
@iegik
Copy link
Author

iegik commented Sep 9, 2017

Sorry, @shergin, but it was not my opinion. https://github.com/facebook/react-native/releases/tag/v0.40.0

iOS native headers moved

This change affects all native modules and application code on iOS that refer to react native .h files
After e1577df, Native code on iOS must refer to headers out of the react namespace. Previously the following would work:

#import "RCTUtils.h"
But now all headers have been moved:

#import <React/RCTUtils.h>
This means that all iOS native libraries need a major version bump for RN 0.40. We attempt to minimize changes of this magnitude, and we apologize for any inconvenience caused.

@shergin
Copy link
Contributor

shergin commented Sep 9, 2017

That proposal and work is about .h files, not .m or .mm files.
I just want to say that if you want to fight for this, you have to prepare really good arguments and deep and comprehensive analysis of all possible approaches.

@iegik
Copy link
Author

iegik commented Sep 10, 2017

I notice, if import file once in with <React/ prefix, next import without prefix cause duplication of declaration error, like now with RCTBridge.h #11725
So, if RCTBridge.h was already imported before in some other module with prefix, and here, in *.m file start to importing without prefix, the compilation cause "duplication error".

Solution:

  • Compile React files first
  • Create list of all React files and describe how they should be imported
  • Revert to 0.39.x version of react and use imports without prefix.
  • Fix all imports to imports with <React/ in all files

See also:

Also, there is some *.m(m) files with <React/ imports:

@iegik
Copy link
Author

iegik commented Sep 15, 2017

@javache , please judge

#import "RCTShadowView+Internal.h"
#import "RCTShadowView.h"
#import "RCTUIManagerObserverCoordinator.h"
#import "RCTUtils.h"
Copy link
Author

@iegik iegik Jun 26, 2018

Choose a reason for hiding this comment

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

https://github.com/facebook/react-native/releases/tag/v0.40.0

This change affects all native modules and application code on iOS that refer to react native .h files

After e1577df, Native code on iOS must refer to headers out of the react namespace. Previously the following would work:
#import "RCTUtils.h"
But now all headers have been moved:
#import <React/RCTUtils.h>
This means that all iOS native libraries need a major version bump for RN 0.40. We attempt to minimize changes of this magnitude, and we apologize for any inconvenience caused.

@iegik
Copy link
Author

iegik commented Jun 26, 2018

@shergin not only *.h files, but all, that "refer to react native .h files":

all native modules and application code on iOS that refer to react native .h files

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.

5 participants