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

Suspense #12279

Merged
merged 19 commits into from May 11, 2018
Merged

Suspense #12279

merged 19 commits into from May 11, 2018

Conversation

acdlite
Copy link
Member

@acdlite acdlite commented Feb 24, 2018

source

TODO:

  • Cache invalidation Will work more on simple-cache-provider in a follow up
  • Naming things
  • Remove dependency on Promise.race
  • Dan's bug Add a failing test for nested fallbacks acdlite/react#2
  • Dan's other bug: It appears that suspense in <div hidden> trees is taken into account. I would expect it to be ignored. Preloading and prerendering shouldn’t affect visual indicator, and shouldn’t prevent an already suspended tree from committing when all Promises from non-hidden resolve. [Andrew: I think this is fixed]

Update (April 13)

Rebased on top of update queue changes. Now depends on #12600.

Update (April 23)

#12600 was merged.

@TrySound
Copy link
Contributor

TrySound commented Feb 24, 2018

Awesome! But are you gonna fit this into 16.3?

@gaearon
Copy link
Member

gaearon commented Feb 24, 2018

We'll see, the release has been slipping for a few weeks because some things turned out harder than we thought. Maybe 16.3 will be a bigger one than we thought.

function waitForTimeout(root, ms, suspendedTime) {
setTimeout(() => {
retryOnPromiseResolution(root, suspendedTime);
}, ms);
Copy link
Member

@gaearon gaearon Feb 25, 2018

Choose a reason for hiding this comment

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

I bumped into a funny case where I accidentally clobbered the timeout queue with a bunch of setTimeouts that reschedule themselves, and the fallback never showed up for this reason. Was very tricky to debug.

false,
'An update was suspended for longer than the timeout, but no fallback ' +
'UI was provided.',
);
Copy link
Member

@gaearon gaearon Feb 25, 2018

Choose a reason for hiding this comment

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

Can we provide a component stack trace here?

@@ -894,10 +964,20 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
} else {
// The root did not complete.
invariant(
false,
!nextRenderIsExpired,
'Expired work should have completed. This error is likely caused ' +
Copy link
Member

@gaearon gaearon Feb 25, 2018

Choose a reason for hiding this comment

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

FYI, I got here with 16.4.0-alpha.3174632

Copy link
Member Author

@acdlite acdlite Feb 25, 2018

Choose a reason for hiding this comment

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

Yeah that’s the bug @ryanflorence found. Should be fixed now, haven’t pushed a new canary yet.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

The more I think of it, the more I’m convinced that having the <Loading /> component wrap both the “from” screen and the “to” screen is not just a convenience but active bad pattern.

Because you start out using it by hoisting it up to a shared parent and it just works. Then you realize that you need two different loading states on the same screen depending on what the target is and you have no way of breaking this up.

It is also really impossible to have a loading state when you’re navigating to a completely different part of the app.

I wonder if we’re modeling the loading state wrong. It seems to me that it is more likely to be close to the setState that triggered the transition.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Feb 25, 2018

Can we split out the Timeout into a separate PR that we can land while we figure out the Loading one?

@acdlite acdlite force-pushed the suspense branch 2 times, most recently from f215727 to 7166ce6 Compare Feb 27, 2018
@acdlite
Copy link
Member Author

acdlite commented Feb 27, 2018

@sebmarkbage Removed the soft expiration stuff. I'll open a separate PR for that. This one is Timeout-only.

@ohyeahgotit
Copy link

ohyeahgotit commented Feb 27, 2018

It already makes sense to me !!!

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Let's see if we can do this without state and update queues on the component level.

I think retrying can happen on the whole root level for the timed out work.

For calling .then we can just do that eagerly and retry for each of them.

return bailoutOnAlreadyFinishedWork(current, workInProgress);
}

if ((workInProgress.effectTag & DidCapture) !== NoEffect) {
Copy link
Collaborator

@sebmarkbage sebmarkbage Feb 27, 2018

Choose a reason for hiding this comment

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

Ignoring legacy context you’re going to check this twice. Use a temp variable here.

renderIsExpired,
remainingTimeMs,
renderStartTime,
renderExpirationTime,
Copy link
Collaborator

@sebmarkbage sebmarkbage Feb 27, 2018

Choose a reason for hiding this comment

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

These extra args are all unused.

Copy link
Member Author

@acdlite acdlite Feb 27, 2018

Choose a reason for hiding this comment

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

Yeah sorry. Soft expiration vestigia.

@ohyeahgotit
Copy link

ohyeahgotit commented Feb 27, 2018

Please tell me what suspense and what will all make sense ??

@acdlite
Copy link
Member Author

acdlite commented Feb 28, 2018

@ohyeahgotit If you're curious, you can watch Dan's talk this Thursday at JSConf Iceland.

@@ -37,3 +37,4 @@ export const Fragment = 10;
export const Mode = 11;
export const ContextConsumer = 12;
export const ContextProvider = 13;
export const TimeoutComponent = 15;
Copy link
Contributor

@nihgwu nihgwu Feb 28, 2018

Choose a reason for hiding this comment

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

should be 14 here for consistence and also added to TypeOfWork above?

Copy link

@stipsan stipsan Feb 28, 2018

Choose a reason for hiding this comment

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

It's probably because @acdlite redacted LoadingComponent to keep us in suspense until @gaearon's talk 🤫

@acdlite acdlite changed the title [WIP] This will all make sense, soon [WIP] Suspense Mar 1, 2018
@mobilespace
Copy link

mobilespace commented Mar 1, 2018

well played @acdlite 👏

lol

@Nimelrian
Copy link

Nimelrian commented Mar 1, 2018

Reactiflux's reaction to Dan's talk (IO Demo).

image

@bpartridge
Copy link

bpartridge commented Mar 1, 2018

For anyone just following this on Github, you can see a roundup of links to the talk and implementation comments here: https://news.ycombinator.com/item?id=16492973

And the money image:

screenshot 2018-03-01 09 41 11

@goldenice
Copy link

goldenice commented Mar 4, 2018

I am a very big fan of this API, I think the whole idea behind it is superb, kudo's to everyone involved! 👍

One thing I was wondering about though: it seems that the deferred setState is the only way to use the suspense feature. Am I correct in assuming that? Because that would mean that a update triggered per prop would not work, which would be bad news for Redux-heavy applications. Effectively that would limit the scope of to within presentational components: everything 'above' a connected component in the tree couldn't use this.

@gaearon
Copy link
Member

gaearon commented Mar 4, 2018

it seems that the deferred setState is the only way to use the suspense feature. Am I correct in assuming that?

Not quite. You can use it with regular setState but it will have a very small expiration time. So it’ll be like if you set the Placeholder timeout to a low value, and will switch to loading state very quickly.

Because that would mean that a update triggered per prop would not work, which would be bad news for Redux-heavy applications

We don’t even have a real API for doing deferred updates yet so this is a premature conclusion.

The real problem with Redux is it only has a concept of one current state. But with suspense you have two. React can handle this but Redux currently doesn’t. This is a much bigger problem that needs to be solved.

We have some ideas on how we can make suspense work with Redux but it’ll take some time and likely some API changes. The issue you pointed out is just a small part of the problem.

@Nimelrian
Copy link

Nimelrian commented Mar 4, 2018

Quick question: if my render calls 3 fetchers, each of which doesn't have the requested value cached, how long would it take at least to render my component when each fetcher takes 500ms to get the data? From what I can tell they are executed in sequence, not parallel, so it would take at least 1500ms (just for the fetching).

@gaearon
Copy link
Member

gaearon commented Mar 4, 2018

Yes, calling multiple fetchers from one component when they don’t strictly depend on one another can be considered a bad practice. You can solve this either by putting them in sibling components (they are parallel) or by using a combinator similar to Promise.all. We might provide one in the final API.

Edit: Nope, I’m wrong #12279 (comment)

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Mar 4, 2018

@gaearon No that is fine practice. Using a Promise.all might in fact be bad practice since it doesn’t let individual values share the cache.

The solution is to use preload.

preload(a);
preload(b);
return read(a) + read(b);

Where preload should ideally be hoisted even further up if possible.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Mar 5, 2018

Another problem with Promise.all is that people tend to think that it’s good to “batch” work. That often leads to a situation where everything is hoisted to a top Promise.all and you can’t do any work until all dependencies are satisfied. That leads to worse parallelization of CPU work.

The preload technique avoids that problem but does require adding code to two places. However, tooling should be able to provide good hints.

@acdlite
Copy link
Member Author

acdlite commented May 10, 2018

Things to address in follow-up PRs:

Immediately...

  • Switching back from a placeholder to the normal view should also be suspendy, not immediately time out. I had this working properly at one point but must not have tested it properly.
  • Round the user provided timeouts to the nearest "just noticeable" threshold, taking into account how much time has already elapsed.
  • Speculatively render the placeholder view immediately upon suspending so it is ready to commit upon timeout. We don't need proper resuming because we can complete the entire root. This should be unified with how root.createBatch works.

After that...

  • Consider renaming Timeout to Placeholder.
  • Consider adding hidden support to fragments so we can hide a timed out tree without losing state and without adding superfluous host nodes. Also consider adding this feature to the built-in placeholder primitive.
  • Improvements to simple-cache-provider
    • Per-key invalidation
    • Subscriptions
    • Garbage collection

At this point I think we'll have an MVP. Then...

Future

  • Implement "soft" expiration. Probably something like ReactDOM.loadingUpdates(batch, onSuspend, onResume)

bucketSizeMs / UNIT_SIZE,
return (
MAGIC_NUMBER_OFFSET +
round(
Copy link
Collaborator

@sebmarkbage sebmarkbage May 10, 2018

Choose a reason for hiding this comment

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

Why this change? What happens if you move this to a follow up?

Copy link
Member Author

@acdlite acdlite May 10, 2018

Choose a reason for hiding this comment

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

When I made the change to infer the start time from the expiration time, all the numbers in the tests were off by 250ms (the bucket size) because we were rounding up. Since in practice it doesn’t really matter if we round up or down when placing updates into buckets, I adjusted it here rather than change all the tests and introduce more noise to the diff.


import {enableSuspense} from 'shared/ReactFeatureFlags';

// TODO: Offscreen updates
Copy link
Collaborator

@sebmarkbage sebmarkbage May 10, 2018

Choose a reason for hiding this comment

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

What is this? It doesn't seem to be on your follow up list.

@@ -28,6 +28,13 @@ export type FiberRoot = {
pendingChildren: any,
// The currently active root fiber. This is the mutable root of the tree.
current: Fiber,

Copy link
Collaborator

@sebmarkbage sebmarkbage May 10, 2018

Choose a reason for hiding this comment

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

Comments plz. What are these for?

Copy link
Member Author

@acdlite acdlite May 10, 2018

Choose a reason for hiding this comment

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

Ok I’ll add comments in the morning

}
}

export function flushPendingPriorityLevel(
Copy link
Collaborator

@sebmarkbage sebmarkbage May 10, 2018

Choose a reason for hiding this comment

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

Flush means something else in the scheduler context. This is more like update something based on something else flushing. This is not the flush.

Copy link
Member Author

@acdlite acdlite May 10, 2018

Choose a reason for hiding this comment

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

Huh this is called after a commit, so “flush” seemed appropriate. Suggestions?

if (current !== null && current.memoizedState === true) {
// A parent Timeout already committed in a placeholder state. We
// need to handle this promise immediately. In other words, we
// should never suspend inside a tree that already expired.
Copy link
Collaborator

@sebmarkbage sebmarkbage May 10, 2018

Choose a reason for hiding this comment

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

So this is the thing you're going to fix in the follow up?

Copy link
Member Author

@acdlite acdlite May 10, 2018

Choose a reason for hiding this comment

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

Yeah, this is the first bullet

thenable.then(onResolveOrReject, onResolveOrReject);
return;
} else {
// No time remaining. Need to fallback to palceholder.
Copy link
Collaborator

@sebmarkbage sebmarkbage May 10, 2018

Choose a reason for hiding this comment

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

palceholder -> placeholder

@@ -28,6 +28,13 @@ export type FiberRoot = {
pendingChildren: any,
// The currently active root fiber. This is the mutable root of the tree.
current: Fiber,

earliestPendingTime: ExpirationTime,
latestPendingTime: ExpirationTime,
Copy link
Collaborator

@sebmarkbage sebmarkbage May 10, 2018

Choose a reason for hiding this comment

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

I don't get why we need to keep track of these two pending times.

Why isn't earliestPendingTime just root.current.expirationTime and latestPendingTime just earliestSuspendedTime - 1? Effectively.

Copy link
Member Author

@acdlite acdlite May 10, 2018

Choose a reason for hiding this comment

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

The pending times are levels we haven’t tried yet. If we try them and they suspend, they are cleared. In other words, need to distinguish between “incomplete” and “incomplete and suspended.”

acdlite and others added 18 commits May 10, 2018
Adds Timeout component. If a promise is thrown from inside a Timeout component,
React will suspend the in-progress render from committing. When the promise
resolves, React will retry. If the render is suspended for longer than the
maximum threshold, the Timeout switches to a placeholder state.

The timeout threshold is defined as the minimum of:
- The expiration time of the current render
- The `ms` prop given to each Timeout component in the ancestor path of the
thrown promise.
Co-authored-by: Andrew Clark <acdlite@fb.com>
React should resume rendering regardless of whether it resolves
or rejects.
Async is not required for Suspense, but strict mode is.
Some of this was added with "soft expiration" in mind, but now with our revised
model for how soft expiration will work, this isn't necessary.

It would be nice to remove more of this, but I think the list itself is inherent
because we need a way to track the start times, for <Timeout ms={ms} />.
Instead of waiting for commit phase.
We can replicate almost all the functionality by tracking just five
separate levels: the highest/lowest priority pending levels, the
highest/lowest priority suspended levels, and the lowest pinged level.

We lose a bit of granularity, in that if there are multiple levels of
pending updates, only the first and last ones are known. But in practice
this likely isn't a big deal.

These heuristics are almost entirely isolated to a single module and
can be adjusted later, without API changes, if necessary.

Non-IO-bound work is not affected at all.
Idk why I thought this was neccessary
This means you have to account for the start time approximation
heuristic when writing Suspense tests, but that's going to be
true regardless.

When updating the tests, I also made a fix related to offscreen
priority. We should never timeout inside a hidden tree.
@acdlite acdlite merged commit 6565795 into facebook:master May 11, 2018
1 check passed
@gaearon
Copy link
Member

gaearon commented May 23, 2018

I don't think <div hidden> is an actual API for this feature. It's currently disabled and we might have a proper API as you suggested later. That said hidden is an actual HTML attribute that denotes something is hidden.

TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
* Timeout component

Adds Timeout component. If a promise is thrown from inside a Timeout component,
React will suspend the in-progress render from committing. When the promise
resolves, React will retry. If the render is suspended for longer than the
maximum threshold, the Timeout switches to a placeholder state.

The timeout threshold is defined as the minimum of:
- The expiration time of the current render
- The `ms` prop given to each Timeout component in the ancestor path of the
thrown promise.

* Add a test for nested fallbacks

Co-authored-by: Andrew Clark <acdlite@fb.com>

* Resume on promise rejection

React should resume rendering regardless of whether it resolves
or rejects.

* Wrap Suspense code in feature flag

* Children of a Timeout must be strict mode compatible

Async is not required for Suspense, but strict mode is.

* Simplify list of pending work

Some of this was added with "soft expiration" in mind, but now with our revised
model for how soft expiration will work, this isn't necessary.

It would be nice to remove more of this, but I think the list itself is inherent
because we need a way to track the start times, for <Timeout ms={ms} />.

* Only use the Timeout update queue to store promises, not for state

It already worked this way in practice.

* Wrap more Suspense-only paths in the feature flag

* Attach promise listener immediately on suspend

Instead of waiting for commit phase.

* Infer approximate start time using expiration time

* Remove list of pending priority levels

We can replicate almost all the functionality by tracking just five
separate levels: the highest/lowest priority pending levels, the
highest/lowest priority suspended levels, and the lowest pinged level.

We lose a bit of granularity, in that if there are multiple levels of
pending updates, only the first and last ones are known. But in practice
this likely isn't a big deal.

These heuristics are almost entirely isolated to a single module and
can be adjusted later, without API changes, if necessary.

Non-IO-bound work is not affected at all.

* ReactFiberPendingWork -> ReactFiberPendingPriority

* Renaming method names from "pending work" to "pending priority"

* Get rid of SuspenseThenable module

Idk why I thought this was neccessary

* Nits based on Sebastian's feedback

* More naming nits + comments

* Add test for hiding a suspended tree to unblock

* Revert change to expiration time rounding

This means you have to account for the start time approximation
heuristic when writing Suspense tests, but that's going to be
true regardless.

When updating the tests, I also made a fix related to offscreen
priority. We should never timeout inside a hidden tree.

* palceholder -> placeholder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet