Allow DevTools to toggle Suspense fallbacks #15232
Conversation
|
ReactDOM: size: 0.0%, gzip: 🔺+0.1% Details of bundled changes.Comparing: e5c5935...35cf972 react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
| @@ -71,6 +74,14 @@ export function injectInternals(internals: Object): boolean { | |||
| onCommitFiberUnmount = catchErrors(fiber => | |||
| hook.onCommitFiberUnmount(rendererID, fiber), | |||
| ); | |||
| onCommitFiberRoot = catchErrors(root => | |||
| hook.onCommitFiberRoot(rendererID, root), | |||
| ); | |||
| @@ -1389,6 +1390,10 @@ function updateSuspenseComponent( | |||
| const mode = workInProgress.mode; | |||
| const nextProps = workInProgress.pendingProps; | |||
|
|
|||
| if (shouldSuspend(workInProgress)) { | |||
There was a problem hiding this comment.
Should be wrapped in __DEV__.
| shouldSuspendFiber = catchErrors(fiber => | ||
| hook.shouldSuspendFiber(rendererID, fiber), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Should be wrapped in __DEV__ too.
|
Make sure you test in both sync mode and concurrent mode |
gaearon
force-pushed
the
suspense-devtools
branch
3 times, most recently
from
578f3f0
to
da14cfb
Compare
gaearon
changed the title
gaearon
force-pushed
the
suspense-devtools
branch
from
da14cfb
to
74265ce
Compare
|
I don't think this is sufficient for DevTools to do feature detection. We should probably add a new prop to where we do the injection that signals whether the current version+build supports this override behavior. This way DevTools won't display a confusing button that doesn't work. Looking at the implementation, I'm curious: why not inject a method e.g. |
Yeah.
That could work. However, I wanted to make it so that the condition can be adjusted dynamically, and doesn't necessarily have to be "here's a Set of Fibers". For example it would be nice if we could do a quick check based on some predicate. Leaving it as a method in DevTools hook makes that a bit more flexible. |
|
During an initial render you can have this pause boundaries that didn’t exist yet. Even in a UI that has rendered boundaries, switching boundary can have side-effects that deletes things in the tree you switched out of. |
You'd need the Hook to say "false" before the renderer is injected for that. Which is something we need for Profiler Reload+Start anyway I guess? |
| @@ -1389,6 +1390,12 @@ function updateSuspenseComponent( | |||
| const mode = workInProgress.mode; | |||
| const nextProps = workInProgress.pendingProps; | |||
|
|
|||
| if (__DEV__) { | |||
| if (shouldSuspend(workInProgress)) { | |||
| workInProgress.effectTag |= DidCapture; | |||
There was a problem hiding this comment.
Suggestion: replace this with
throwException(
root,
workInProgress, // returnFiber
null, // sourceFiber
dummyPromiseThatNeverResolves,
renderExpirationTime
);This will require refactoring throwException a bit, or perhaps splitting it into multiple functions, since it currently assumes that you have a source fiber.
EDIT: Never mind, I forgot you also need to call unwindWork in concurrent mode... but not in sync mode.
What motivated this suggestion is that there are a lot of subtle differences to account for between the two modes and I'm paranoid about neglecting one of them.
|
I hadn't realized that you were thinking of supporting a feature like "reload and step through suspense boundaries". Given that, deferring to DevTools does make more sense. I'm not entirely comfortable with the current solution for injection in the reload-and-do-something case, but it's reasonable to assume this could piggy back off of whatever solution we end up with there. |
gaearon
force-pushed
the
suspense-devtools
branch
from
74265ce
to
386917c
Compare
This lets detect support for overriding Suspense from DevTools.
gaearon
force-pushed
the
suspense-devtools
branch
from
386917c
to
efebb67
Compare
|
I updated the PR to a different API. Now it exposes The upside of this is that it makes it easy to feature-test whether a particular React renderer supports this feature from DevTools. Note that while this moves the override to render injection (as opposed to reading a field from the Global Hook), it doesn't actually make it any harder to "pause" the first render. If we wanted, we could call I've also added a CM test. |
What it says. Not sure if this is sufficient yet. I'll build it out a bit more before we get this in.