Skip to content

Add React.Suspense component, remove React.useMutationEffect hook #7213

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

Closed
wants to merge 5 commits into from
Closed

Add React.Suspense component, remove React.useMutationEffect hook #7213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a62cb0
Remove useMutationEffect from React libdef and update tests
Nov 27, 2018
bb4646f
Add React.Suspense component and tests
Nov 27, 2018
6af8901
Make React.Suspense "fallback" prop optional (and add nested Suspense…
Nov 27, 2018
e762f41
Fixed useCallback definition (and tests)
Nov 28, 2018
9b45d2b
Fixed React.lazy definition and tests
Nov 29, 2018
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
19 changes: 10 additions & 9 deletions lib/react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,12 @@ declare module react {
declare export var ConcurrentMode: ({children: ?React$Node}) => React$Node; // 16.7+
declare export var StrictMode: ({children: ?React$Node}) => React$Node;

This comment was marked as resolved.

Sign in to view

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. I'm ok with adding that fix to this PR or doing it in a separate one.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bvaughn actually brought up a good point below, it's not quite a ComponentType. It will be an AbstractComponent, though!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's leave it for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, sounds good


declare export var Suspense: React$ComponentType<{
children?: ?React$Node,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed?
IIUC, this says that it may or may not have children, and they have to be react nodes. I think that's the default.

Copy link
Contributor

Choose a reason for hiding this comment

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

whoa, that seems like a bug – composite components should only take children if it's explicitly specified (it's only host components like 'div' that always take React$Node children)

Copy link
Contributor Author

@bvaughn bvaughn Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Based on the context Provider and Consumer types above, I assumed an explicit children prop was needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, children is the part of our react model that I'm still not totally familiar with. I'm glad we're having this conversation because it's forcing me to look into it more.

@sophiebits: I'll have to do some more investigation. Requiring an explicit children prop would be an extremely disruptive change at this point.

@bvaughn: I just took a look at our react docs to see what we say about children types. It looks like we have conflicting advice here and here. I think the ?React.Node bit is necessary, but I'm unclear about whether or not the ?children part is also necessary. I'll have to look into it more, but either way I'm ok with what you have here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sophiebits: Width subtyping allows extra properties to be passed into an object, so if the Props type is not exact then we allow children even if they are not specified explicitly. On the other hand, if the Props type is exact then we don't allow the extra properties.

Mystery solved!

Copy link
Contributor

Choose a reason for hiding this comment

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

for posterity: React's treatment of children:

  1. React.createElement takes varargs that become props.children (JSX children syntax turns into this)
  2. host component ('div' etc) children prop needs to be react nodes and React recurses into them
  3. composite component children are not special in any way; by convention people often use it to contain React nodes then render <div>{props.children}</div> or similar, but they can also be other data types (eg: the "render props" pattern often has children be a function)

fallback?: React$Node,
maxDuration?: number
}>; // 16.6+
Copy link

Choose a reason for hiding this comment

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

Does this imply that this is a class? I guess that isn't technically correct since it's one of those internal special symbols like Fragment but maybe good enough for now.

Should it use the same hack as Fragments and pretend to be a functional component? That way maybe refs are invalid maybe?

Copy link
Contributor Author

@bvaughn bvaughn Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Should it use the same hack as Fragments and pretend to be a functional component? That way maybe refs are invalid maybe?

Then people might think they can call it as a function, which would error? (Doesn't seem like that would obviously be better than this.)

Copy link
Contributor

Choose a reason for hiding this comment

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

ComponentType is the type alias that (currently) represents function components and class components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there an alias for something that more accurately represents StrictMode, Suspense, etc?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not yet, but AbstractComponent will fulfill that role!

Copy link
Contributor

Choose a reason for hiding this comment

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

Then people might think they can call it as a function, which would error? (Doesn't seem like that would obviously be better than this.)

If the instance type is void, it's more precise to use the function hack. Since we're already using that hack today, and since it will be fixed as part of my work on AbstractComponent, I think we should use the function representation for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must be missing something, but changing it to a function declaration seems break the validation.

For example, this test fails, as I would expect.

But this one and this one do not.

Copy link
Contributor Author

@bvaughn bvaughn Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Calling Suspense directly (as a plain function) e.g. Suspense({ maxDuration: 'abc' }) properly catches the error, but wrapping it in a createElement call e.g. <Suspense maxDuration="abc" /> does not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fragment seems similarly broken? Shouldn't this fail?

https://flow.org/try/#0JYWwDg9gTgLgBAJQKYEMDG8BmUIjgcilQ3wG4AoczAVwDsNgJa4AxCCACgEo4BvOIjGpRmtagBtxcAL6UAPMnQwAdCygoA5iCS0YAPl5sI0uQHpFGVeq0795IA


declare export type ElementProps<C> = React$ElementProps<C>;
declare export type ElementConfig<C> = React$ElementConfig<C>;
declare export type ElementRef<C> = React$ElementRef<C>;
Expand Down Expand Up @@ -285,7 +291,7 @@ declare module react {
): React$StatelessFunctionalComponent<P>;

declare export function lazy<P>(
component: () => React$ComponentType<P>,
component: () => Promise<{ default: React$ComponentType<P> }>,
): React$ComponentType<P>;

declare type MaybeCleanUpFn = ?(() => mixed);
Expand All @@ -312,18 +318,13 @@ declare module react {
inputs: ?$ReadOnlyArray<mixed>,
): void;

declare export function useMutationEffect(
create: () => MaybeCleanUpFn,
inputs: ?$ReadOnlyArray<mixed>,
): void;

declare export function useLayoutEffect(
create: () => MaybeCleanUpFn,
inputs: ?$ReadOnlyArray<mixed>,
): void;

declare export function useCallback<T>(
callback: () => T | void,
declare export function useCallback<T: (...args: $ReadOnlyArray<empty>) => mixed>(
callback: T,
inputs: ?$ReadOnlyArray<mixed>,
): T;

Expand Down Expand Up @@ -358,12 +359,12 @@ declare module react {
+Children: typeof Children,
+ConcurrentMode: typeof ConcurrentMode,
+StrictMode: typeof StrictMode,
+Suspense: typeof Suspense,
+useContext: typeof useContext,
+useState: typeof useState,
+useReducer: typeof useReducer,
+useRef: typeof useRef,
+useEffect: typeof useEffect,
+useMutationEffect: typeof useMutationEffect,
+useLayoutEffect: typeof useLayoutEffect,
+useCallback: typeof useCallback,
+useMemo: typeof useMemo,
Expand Down
8 changes: 4 additions & 4 deletions tests/getters_and_setters/getters_and_setters.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,8 +465,8 @@ References:
react.js:17:13
17| (<Example a="bad" />); // error: number ~> string
^^^^^ [1]
<BUILTINS>/react.js:410:36
410| number: React$PropType$Primitive<number>;
<BUILTINS>/react.js:411:36
411| number: React$PropType$Primitive<number>;
^^^^^^ [2]


Expand All @@ -482,8 +482,8 @@ References:
react.js:18:20
18| (<Example a={0} c={0} />); // error: number ~> string
^ [1]
<BUILTINS>/react.js:412:36
412| string: React$PropType$Primitive<string>;
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [2]


Expand Down
46 changes: 23 additions & 23 deletions tests/new_react/new_react.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,8 @@ Cannot assign `this.props.x` to `_` because number [1] is incompatible with stri
^^^^^^^^^^^^

References:
<BUILTINS>/react.js:410:36
410| number: React$PropType$Primitive<number>;
<BUILTINS>/react.js:411:36
411| number: React$PropType$Primitive<number>;
^^^^^^ [1]
classes.js:57:12
57| var _: string = this.props.x;
Expand Down Expand Up @@ -388,8 +388,8 @@ Cannot assign `this.props.z` to `qux` because:
^^^^^^^^^^^^

References:
<BUILTINS>/react.js:410:36
410| number: React$PropType$Primitive<number>;
<BUILTINS>/react.js:411:36
411| number: React$PropType$Primitive<number>;
^^^^^^ [1]
new_react.js:19:18
19| var qux: string = this.props.z;
Expand All @@ -405,8 +405,8 @@ Cannot assign `this.props.x` to `w` because string [1] is incompatible with numb
^^^^^^^^^^^^

References:
<BUILTINS>/react.js:412:36
412| string: React$PropType$Primitive<string>;
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [1]
new_react.js:20:15
20| var w:number = this.props.x;
Expand Down Expand Up @@ -439,8 +439,8 @@ References:
new_react.js:29:23
29| var element = <C x = {0}/>;
^ [1]
<BUILTINS>/react.js:412:36
412| string: React$PropType$Primitive<string>;
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [2]


Expand Down Expand Up @@ -545,8 +545,8 @@ Cannot assign `this.props.x` to `a` because:
^^^^^^^^^^^^

References:
<BUILTINS>/react.js:412:36
412| string: React$PropType$Primitive<string>;
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [1]
props.js:14:16
14| var a: number = this.props.x; // error
Expand Down Expand Up @@ -582,8 +582,8 @@ Cannot assign `this.props.z` to `c` because:
^^^^^^^^^^^^

References:
<BUILTINS>/react.js:410:36
410| number: React$PropType$Primitive<number>;
<BUILTINS>/react.js:411:36
411| number: React$PropType$Primitive<number>;
^^^^^^ [1]
props.js:16:16
16| var c: string = this.props.z; // error
Expand All @@ -604,14 +604,14 @@ References:
props.js:20:29
20| var element = <TestProps x={false} y={false} z={false} />; // 3 errors
^^^^^ [1]
<BUILTINS>/react.js:412:36
412| string: React$PropType$Primitive<string>;
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [2]
props.js:20:49
20| var element = <TestProps x={false} y={false} z={false} />; // 3 errors
^^^^^ [3]
<BUILTINS>/react.js:410:36
410| number: React$PropType$Primitive<number>;
<BUILTINS>/react.js:411:36
411| number: React$PropType$Primitive<number>;
^^^^^^ [4]


Expand Down Expand Up @@ -668,8 +668,8 @@ References:
props2.js:9:41
9| getInitialState: function(): { bar: number } {
^^^^^^ [1]
<BUILTINS>/react.js:412:36
412| string: React$PropType$Primitive<string>;
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [2]
props2.js:15:42
15| return <C {...this.state} foo = {0} />;
Expand Down Expand Up @@ -700,12 +700,12 @@ Cannot get `React.PropTypes.string.inRequired` because property `inRequired` is
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

References:
<BUILTINS>/react.js:386:39
<BUILTINS>/react.js:387:39
v
386| type ReactPropsChainableTypeChecker = {
387| isRequired: ReactPropsCheckType;
388| (props: any, propName: string, componentName: string, href?: string): ?Error;
389| };
387| type ReactPropsChainableTypeChecker = {
388| isRequired: ReactPropsCheckType;
389| (props: any, propName: string, componentName: string, href?: string): ?Error;
390| };
^ [1]


Expand Down
Loading