[React Native] measure calls will now call FabricUIManager #15324
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Fabric renderer was previously calling the paper UIManager's measure calls and passing the react tag. This PR changes the renderer to now call FabricUIManager passing the node instead. One of the parts of this that feels more controversial is making NativeMethodsMixin and ReactNative.NativeComponent warn when calling measureLayout in Fabric. As Seb and I decided in facebook#15126, it doesn't make sense for a component created with one of these methods to require a native ref but not work the other way around. For example: a.measureLayout(b) might work but b.measureLayout(a) wouldn't. We figure we should keep these consistent and continue migrating things off of NativeMethodsMixin and NativeComponent. If this becomes problematic for the Fabric rollout then we should revisit this.
Details of bundled changes.Comparing: c7a9599...a012aa1 react-native-renderer
Generated by 🚫 dangerJS |
|
I need to track down these lint errors: I think I'm probably missing some define somewhere. |
|
I think this is failing because the To fix this, replace: import FabricUIManager from 'FabricUIManager';with: import * as FabricUIManager from 'FabricUIManager'; |
sebmarkbage
reviewed
Apr 5, 2019
| @@ -89,7 +89,7 @@ class ReactNativeComponent<Props> extends React.Component<Props> { | |||
| measure(callback: MeasureOnSuccessCallback): void {} | |||
| measureInWindow(callback: MeasureInWindowOnSuccessCallback): void {} | |||
| measureLayout( | |||
| relativeToNativeNode: number, | |||
| relativeToNativeNode: number | Object, | |||
There was a problem hiding this comment.
Let's make this number | ReactNativeComponent and cast it on the inside. It's not correct but more specific.
sebmarkbage
approved these changes
Apr 5, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is based on #15323, split for review.
The Fabric renderer was previously calling the paper UIManager's measure calls and passing the react tag. This PR changes the renderer to now call FabricUIManager passing the node instead.
In #15126 Seb and I were thinking that we shouldn't support components created with NativeMethodsMixin and ReactNative.NativeComponent to be able to take a ref in measureLayout.
While that is still ideal, we can't reasonably make that change because the signatures of FabricHostComponent, FiberHostComponent, NativeMethodsMixin, and ReactNativeNativeComponent all have identical type signatures as enforced in each file with this:
Ideally we'd type host components differently than the other methods so that we can use flow to enforce that an argument is a ref to a host component and not one of the others. Unfortunately, we are pretty far from being able to do that today because all the core components in React Native are typed like this:
We will need to fix this and update all the ref types throughout the codebase to be more strict in order to properly type the argument to measureLayout as a ref to a host component. This is a much bigger project and out of scope of this PR. We should make that change at some point.
In order to keep all the types consistent in this PR measureLayout is taking a reactTag or object and the checks are done at runtime.
The additions to the FabricUIManager types are consistent with D14732142.