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.

Sign up for GitHub

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

Add HostComponent type to ReactNative #16898

Merged
merged 3 commits into from
Sep 26, 2019
Merged

Add HostComponent type to ReactNative #16898

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
afea710
Add HostComponent type to ReactNative
elicwhite Sep 25, 2019
e99175b
Use type alias imports instead of wildcard
elicwhite Sep 25, 2019
db60405
Fix forgotten Object in measureLayout type
elicwhite Sep 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-native-renderer/src/NativeMethodsMixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ export default function(
measureLayout: function(
relativeToNativeNode: number | Object,
onSuccess: MeasureLayoutOnSuccessCallback,
onFail: () => void /* currently unused */,
onFail?: () => void /* currently unused */,
Copy link
Member Author

Choose a reason for hiding this comment

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

This was always optional in the types used in React Native, but because the two types are duplicated in ReactNativeTypes, they must have fallen out of sync.

) {
let maybeInstance;

Expand Down
8 changes: 4 additions & 4 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import type {
MeasureInWindowOnSuccessCallback,
MeasureLayoutOnSuccessCallback,
MeasureOnSuccessCallback,
NativeMethodsMixinType,
NativeMethods,
ReactNativeBaseComponentViewConfig,
ReactNativeResponderEvent,
ReactNativeResponderContext,
Expand Down Expand Up @@ -151,9 +151,9 @@ class ReactFabricHostComponent {
}

measureLayout(
relativeToNativeNode: number | Object,
Copy link
Member Author

Choose a reason for hiding this comment

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

Yay!

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize it's not part of this change, but should the warningWithoutStack inside of this function (and the one in packages/react-native-renderer/src/ReactNativeFiberHostComponent.js) be __DEV__ gated?

Copy link
Member Author

Choose a reason for hiding this comment

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

warningWithoutStack internally has a dev check and is a no-op in dev.

relativeToNativeNode: number | ReactFabricHostComponent,
onSuccess: MeasureLayoutOnSuccessCallback,
onFail: () => void /* currently unused */,
onFail?: () => void /* currently unused */,
) {
if (
typeof relativeToNativeNode === 'number' ||
Expand Down Expand Up @@ -186,7 +186,7 @@ class ReactFabricHostComponent {
}

// eslint-disable-next-line no-unused-expressions
(ReactFabricHostComponent.prototype: NativeMethodsMixinType);
(ReactFabricHostComponent.prototype: NativeMethods);

export * from 'shared/HostConfigWithNoMutation';
export * from 'shared/HostConfigWithNoHydration';
Expand Down
6 changes: 3 additions & 3 deletions packages/react-native-renderer/src/ReactNativeComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type {
MeasureInWindowOnSuccessCallback,
MeasureLayoutOnSuccessCallback,
MeasureOnSuccessCallback,
NativeMethodsMixinType,
NativeMethods,
ReactNativeBaseComponentViewConfig,
} from './ReactNativeTypes';

Expand Down Expand Up @@ -172,7 +172,7 @@ export default function(
measureLayout(
relativeToNativeNode: number | Object,
onSuccess: MeasureLayoutOnSuccessCallback,
onFail: () => void /* currently unused */,
onFail?: () => void /* currently unused */,
): void {
let maybeInstance;

Expand Down Expand Up @@ -295,7 +295,7 @@ export default function(
}

// eslint-disable-next-line no-unused-expressions
(ReactNativeComponent.prototype: NativeMethodsMixinType);
(ReactNativeComponent.prototype: NativeMethods);

return ReactNativeComponent;
}
14 changes: 9 additions & 5 deletions packages/react-native-renderer/src/ReactNativeFiberHostComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import type {
MeasureInWindowOnSuccessCallback,
MeasureLayoutOnSuccessCallback,
MeasureOnSuccessCallback,
NativeMethodsMixinType,
NativeMethods,
ReactNativeBaseComponentViewConfig,
} from './ReactNativeTypes';
import type {Instance} from './ReactNativeHostConfig';
Expand Down Expand Up @@ -72,21 +72,25 @@ class ReactNativeFiberHostComponent {
}

measureLayout(
relativeToNativeNode: number | Object,
relativeToNativeNode: number | ReactNativeFiberHostComponent,
onSuccess: MeasureLayoutOnSuccessCallback,
onFail: () => void /* currently unused */,
onFail?: () => void /* currently unused */,
) {
let relativeNode;
let relativeNode: ?number;

if (typeof relativeToNativeNode === 'number') {
// Already a node handle
relativeNode = relativeToNativeNode;
} else if (relativeToNativeNode._nativeTag) {
relativeNode = relativeToNativeNode._nativeTag;
} else if (
/* $FlowFixMe canonical doesn't exist on the node.
I think this branch is dead and will remove it in a followup */
relativeToNativeNode.canonical &&
relativeToNativeNode.canonical._nativeTag
) {
/* $FlowFixMe canonical doesn't exist on the node.
I think this branch is dead and will remove it in a followup */
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if the change from Object -> ReactNativeFiberHostComponent made sense given relativeToNativeNode.canonical but I assume it's okay?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah.

When I added this option I wrote tests that called this with createClass, ReactNativeNativeComponent and a host component. Nothing triggers this branch. That's why I think this is dead.

Worst case, nothing except some of our Fabric screens are using this API yet and we'll get quick signal if I'm wrong once we delete it. But it's unrelated to this type change.

relativeNode = relativeToNativeNode.canonical._nativeTag;
}

Expand Down Expand Up @@ -137,6 +141,6 @@ class ReactNativeFiberHostComponent {
}

// eslint-disable-next-line no-unused-expressions
(ReactNativeFiberHostComponent.prototype: NativeMethodsMixinType);
(ReactNativeFiberHostComponent.prototype: NativeMethods);

export default ReactNativeFiberHostComponent;
19 changes: 15 additions & 4 deletions packages/react-native-renderer/src/ReactNativeTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
* @flow
*/

import React from 'react';
import * as React from 'react';
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed because we are now importing types from React. ElementRef, and AbstractComponent.


export type MeasureOnSuccessCallback = (
x: number,
Expand Down Expand Up @@ -96,23 +96,34 @@ class ReactNativeComponent<Props> extends React.Component<Props> {
setNativeProps(nativeProps: Object): void {}
}

// This type is only used for FlowTests. It shouldn't be imported directly
export type _InternalReactNativeComponentClass<Props> = Class<
ReactNativeComponent<Props>,
>;

/**
* This type keeps ReactNativeFiberHostComponent and NativeMethodsMixin in sync.
* It can also provide types for ReactNative applications that use NMM or refs.
*/
export type NativeMethodsMixinType = {
export type NativeMethods = {
blur(): void,
focus(): void,
measure(callback: MeasureOnSuccessCallback): void,
measureInWindow(callback: MeasureInWindowOnSuccessCallback): void,
measureLayout(
relativeToNativeNode: number | Object,
relativeToNativeNode: number | React.ElementRef<HostComponent<mixed>>,
onSuccess: MeasureLayoutOnSuccessCallback,
onFail: () => void,
onFail?: () => void,
): void,
setNativeProps(nativeProps: Object): void,
};

export type NativeMethodsMixinType = NativeMethods;
export type HostComponent<T> = React.AbstractComponent<
Copy link
Member Author

Choose a reason for hiding this comment

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

So simple. So clean.

T,
$ReadOnly<$Exact<NativeMethods>>,
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 I understand the purpose of $ReadOnly or $Exact here.

Copy link
Member Author

Choose a reason for hiding this comment

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

From @jbrown215:

Definitely consider making the instance type of HostComponent read-only-- all of those function names seem like things you don't want people to overwrite. If the object really has no other properties, you can also make it exact. If it might, don't, otherwise spreading that object would be unsound (not that I can see a reason why someone would want to spread it?)

>;

type SecretInternalsType = {
NativeMethodsMixin: NativeMethodsMixinType,
computeComponentStackForErrorReporting(tag: number): string,
Expand Down