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

bvaughn · 2018-11-27T21:55:46Z

Resolves #7219

2fdcda5: Removed React.useMutationEffect hook (Remove useMutationEffect react#14336) and updated tests. This hook was only ever exposed in an alpha release of 16.7 and so it should be safe to remove in a non-backwards-compatible way.
dabf49e: Add React.Suspense component and tests. (No final public documentation exists for Suspense yet– although we're working on it. For the time being, tests like this can serve as a reference.)
8df05bd: Fixed invalid useCallback definition and tests.
063b022: Fixed invalid React.lazy signature to reflect that a Promise with a "default" property should be returned (rather than a React component).

bvaughn · 2018-11-27T21:57:33Z

The type-at-pos test kept crashing for me locally but I don't think that's related to this PR.

facebook-github-bot

@jbrown215 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

jbrown215 · 2018-11-27T22:01:23Z

lib/react.js

@@ -250,6 +250,12 @@ declare module react {
  declare export var ConcurrentMode: ({children: ?React$Node}) => React$Node; // 16.7+
  declare export var StrictMode: ({children: ?React$Node}) => React$Node;


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

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

Let's leave it for now?

Yeah, sounds good

sophiebits · 2018-11-27T21:58:00Z

lib/react.js

@@ -250,6 +250,12 @@ declare module react {
  declare export var ConcurrentMode: ({children: ?React$Node}) => React$Node; // 16.7+
  declare export var StrictMode: ({children: ?React$Node}) => React$Node;

+  declare export var Suspense: React$ComponentType<{
+    children?: ?React$Node,
+    fallback: React$Node,


fallback is optional (for nested Suspense components); missing, undefined, and null should all typecheck I think

Ah thanks! I thought it was a required prop.

sebmarkbage · 2018-11-27T21:59:47Z

lib/react.js

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


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?

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.)

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

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

Not yet, but AbstractComponent will fulfill that role!

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.

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.

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.

Fragment seems similarly broken? Shouldn't this fail?

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

jbrown215 · 2018-11-27T22:16:45Z

lib/react.js

@@ -250,6 +250,12 @@ declare module react {
  declare export var ConcurrentMode: ({children: ?React$Node}) => React$Node; // 16.7+
  declare export var StrictMode: ({children: ?React$Node}) => React$Node;

+  declare export var Suspense: React$ComponentType<{
+    children?: ?React$Node,


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.

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)

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

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.

@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!

for posterity: React's treatment of children:

React.createElement takes varargs that become props.children (JSX children syntax turns into this)

host component ('div' etc) children prop needs to be react nodes and React recurses into them

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)

bvaughn · 2018-11-27T22:30:52Z

I pushed a change to make Suspense "fallback" prop optional but I didn't change the component type yet because it looks broken.

jbrown215 · 2018-11-28T14:59:08Z

Yea, that looks like a bug to me.

The only downside I see with ComponentType is that we won't correctly check the ref pseudo-prop. Other than that, it's just a strange representation. I'd rather not block people from using it just because it's strange and has unsafe ref typing. I'm ok with this as is.

AbstractComponent is far enough along that I don't think this problem will be around for long.

facebook-github-bot

@jbrown215 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

bvaughn · 2018-11-28T17:58:25Z

Sweet! Thanks for the udpate!

jbrown215 · 2018-11-28T22:11:01Z

@bvaughn: I think the tests need to be re-recorded again.

bvaughn · 2018-11-28T22:12:09Z

Yeah, I noticed. I guess something landed in master that caused conflicts.

I'm in the process of rebasing and rerecording now.

bvaughn · 2018-11-28T22:21:21Z

I noticed something wrong with the useCallback def that I'm going to go ahead and toss in here too.

bvaughn · 2018-11-28T22:32:28Z

Okay. I think I fixed the bad useCallback definition (and tests) via e33b4b5. Another round of review, pls!

Edit not sure why CI is failing:

=== ERROR while compiling core_kernel.v0.11.1 ================================#
context 2.0.0 | win32/x86_64 | ocaml-variants.4.05.0+mingw64c | git+https://github.com/fdopen/opam-repository-mingw.git#opam2

bvaughn · 2018-11-29T00:02:04Z

Okay. useCallback updated (again) as well as lazy. Let's see if CI is happier.

facebook-github-bot

@jbrown215 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

TrySound · 2018-11-29T00:12:12Z

Can useRef current be not nullable since it has default value?

jbrown215 · 2018-11-29T00:13:25Z

The default value isn't required :(. This is something Brian and I did discuss too

… test cases)

bvaughn · 2018-11-29T15:40:28Z

FB internal lint failure because of the lack of a newline at the end of the lazy test :( so I rebased once more and fixed that along the way.

facebook-github-bot

@bvaughn has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Resolves #7219 - [x] 2fdcda5: Removed `React.useMutationEffect` hook (facebook/react/pull/14336) and updated tests. This hook was only ever exposed in an alpha release of 16.7 and so it should be safe to remove in a non-backwards-compatible way. - [x] dabf49e: Add `React.Suspense` component and tests. (No final public documentation exists for `Suspense` yet– although we're working on it. For the time being, [tests like this](https://github.com/facebook/react/blob/master/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js) can serve as a reference.) - [x] 8df05bd: Fixed invalid `useCallback` definition and tests. - [x] 063b022: Fixed invalid `React.lazy` signature to reflect that a `Promise` with a "default" property should be returned (rather than a React component). Pull Request resolved: #7213 Reviewed By: jbrown215 Differential Revision: D13222880 Pulled By: bvaughn fbshipit-source-id: 9e1b48ace984051928c4ab92fa61182ad70f7119

facebook-github-bot added the CLA Signed label Nov 27, 2018

facebook-github-bot reviewed Nov 27, 2018

View reviewed changes

sophiebits reviewed Nov 27, 2018

View reviewed changes

sebmarkbage reviewed Nov 27, 2018

View reviewed changes

jbrown215 reviewed Nov 27, 2018

View reviewed changes

facebook-github-bot reviewed Nov 28, 2018

View reviewed changes

bvaughn force-pushed the react branch from a80397e to e33b4b5 Compare November 28, 2018 22:32

bvaughn force-pushed the react branch from e33b4b5 to 063b022 Compare November 29, 2018 00:01

facebook-github-bot reviewed Nov 29, 2018

View reviewed changes

Brian Vaughn added 5 commits November 29, 2018 07:39

Remove useMutationEffect from React libdef and update tests

0a62cb0

Add React.Suspense component and tests

bb4646f

Make React.Suspense "fallback" prop optional (and add nested Suspense…

6af8901

… test cases)

Fixed useCallback definition (and tests)

e762f41

Fixed React.lazy definition and tests

9b45d2b

bvaughn force-pushed the react branch from 063b022 to 9b45d2b Compare November 29, 2018 15:40

facebook-github-bot reviewed Nov 29, 2018

View reviewed changes

facebook-github-bot closed this in b5c6598 Nov 29, 2018

goodmind added the Merged label Jul 28, 2019

		@@ -250,6 +250,12 @@ declare module react {
		declare export var ConcurrentMode: ({children: ?React$Node}) => React$Node; // 16.7+
		declare export var StrictMode: ({children: ?React$Node}) => React$Node;

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

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

Conversation

bvaughn commented Nov 27, 2018 • edited Loading

bvaughn commented Nov 27, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Nov 27, 2018 • edited Loading

jbrown215 commented Nov 28, 2018 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

bvaughn commented Nov 28, 2018

jbrown215 commented Nov 28, 2018

bvaughn commented Nov 28, 2018

bvaughn commented Nov 28, 2018

bvaughn commented Nov 28, 2018 • edited Loading

bvaughn commented Nov 29, 2018 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

TrySound commented Nov 29, 2018

jbrown215 commented Nov 29, 2018

bvaughn commented Nov 29, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

bvaughn commented Nov 27, 2018 •

edited

Loading

bvaughn Nov 27, 2018 •

edited

Loading

bvaughn Nov 27, 2018 •

edited

Loading

bvaughn Nov 27, 2018 •

edited

Loading

bvaughn commented Nov 27, 2018 •

edited

Loading

jbrown215 commented Nov 28, 2018 •

edited

Loading

bvaughn commented Nov 28, 2018 •

edited

Loading

bvaughn commented Nov 29, 2018 •

edited

Loading