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

Allow DevTools to toggle Suspense fallbacks #15232

Merged
merged 6 commits into from Apr 4, 2019

Conversation

@gaearon
Copy link
Member

commented Mar 28, 2019

What it says. Not sure if this is sufficient yet. I'll build it out a bit more before we get this in.

@sizebot

This comment has been minimized.

Copy link

commented Mar 28, 2019

ReactDOM: size: 0.0%, gzip: 🔺+0.1%

Details of bundled changes.

Comparing: e5c5935...35cf972

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 886.14 KB 886.72 KB 200.15 KB 200.26 KB UMD_DEV
react-dom.production.min.js 0.0% 🔺+0.1% 105.49 KB 105.53 KB 34.15 KB 34.17 KB UMD_PROD
react-dom.profiling.min.js 0.0% +0.1% 108.5 KB 108.54 KB 34.76 KB 34.78 KB UMD_PROFILING
react-dom.development.js +0.1% +0.1% 880.76 KB 881.33 KB 198.59 KB 198.69 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 105.48 KB 105.52 KB 33.62 KB 33.63 KB NODE_PROD
react-dom.profiling.min.js 0.0% 0.0% 108.59 KB 108.64 KB 34.19 KB 34.21 KB NODE_PROFILING
ReactDOM-dev.js +0.1% +0.1% 906.93 KB 907.5 KB 200.48 KB 200.59 KB FB_WWW_DEV
ReactDOM-prod.js 0.0% 0.0% 333.45 KB 333.51 KB 61.36 KB 61.37 KB FB_WWW_PROD
ReactDOM-profiling.js 0.0% 0.0% 340.14 KB 340.2 KB 62.76 KB 62.77 KB FB_WWW_PROFILING
react-dom-unstable-fire.development.js +0.1% +0.1% 886.46 KB 887.04 KB 200.29 KB 200.39 KB UMD_DEV
react-dom-unstable-fire.production.min.js 0.0% 🔺+0.1% 105.51 KB 105.55 KB 34.16 KB 34.18 KB UMD_PROD
react-dom-unstable-fire.profiling.min.js 0.0% +0.1% 108.52 KB 108.56 KB 34.77 KB 34.79 KB UMD_PROFILING
react-dom-unstable-fire.development.js +0.1% +0.1% 881.08 KB 881.66 KB 198.72 KB 198.82 KB NODE_DEV
react-dom-unstable-fire.production.min.js 0.0% 0.0% 105.49 KB 105.53 KB 33.62 KB 33.64 KB NODE_PROD
react-dom-unstable-fire.profiling.min.js 0.0% 0.0% 108.61 KB 108.65 KB 34.2 KB 34.22 KB NODE_PROFILING
ReactFire-dev.js +0.1% +0.1% 906.11 KB 906.69 KB 200.46 KB 200.58 KB FB_WWW_DEV
ReactFire-prod.js 0.0% 0.0% 322.11 KB 322.17 KB 59.02 KB 59.04 KB FB_WWW_PROD
ReactFire-profiling.js 0.0% 0.0% 328.74 KB 328.8 KB 60.42 KB 60.44 KB FB_WWW_PROFILING
react-dom-unstable-new-scheduler.development.js +0.1% +0.1% 879.94 KB 880.51 KB 198.19 KB 198.3 KB NODE_DEV
react-dom-unstable-new-scheduler.production.min.js 0.0% 0.0% 110.64 KB 110.69 KB 35.28 KB 35.3 KB NODE_PROD
ReactDOMNewScheduler-dev.js +0.1% +0.1% 905.83 KB 906.41 KB 200.33 KB 200.44 KB FB_WWW_DEV
ReactDOMNewScheduler-prod.js 0.0% 0.0% 315.38 KB 315.44 KB 58.4 KB 58.42 KB FB_WWW_PROD
ReactDOMNewScheduler-profiling.js 0.0% 0.0% 321.08 KB 321.14 KB 59.48 KB 59.5 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.42 KB 10.42 KB 3.56 KB 3.57 KB NODE_PROD
react-dom-server.browser.development.js 0.0% 0.0% 131.62 KB 131.62 KB 34.69 KB 34.69 KB NODE_DEV
ReactDOMServer-dev.js 0.0% -0.0% 133.88 KB 133.88 KB 34.43 KB 34.43 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% -0.0% 46.62 KB 46.62 KB 10.71 KB 10.71 KB FB_WWW_PROD
react-dom-server.node.development.js 0.0% 0.0% 133.56 KB 133.56 KB 35.24 KB 35.24 KB NODE_DEV

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% +0.1% 635.37 KB 635.95 KB 137.3 KB 137.41 KB UMD_DEV
react-art.production.min.js 0.0% 🔺+0.1% 97.21 KB 97.25 KB 29.75 KB 29.78 KB UMD_PROD
react-art.development.js +0.1% +0.1% 566.56 KB 567.14 KB 120.04 KB 120.14 KB NODE_DEV
react-art.production.min.js 🔺+0.1% 🔺+0.1% 62.21 KB 62.25 KB 19.02 KB 19.03 KB NODE_PROD
ReactART-dev.js +0.1% +0.1% 577.66 KB 578.24 KB 119.67 KB 119.78 KB FB_WWW_DEV
ReactART-prod.js 0.0% 🔺+0.1% 198.05 KB 198.11 KB 33.53 KB 33.55 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.1% +0.1% 699.19 KB 699.77 KB 149.79 KB 149.89 KB RN_FB_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 246.64 KB 246.7 KB 43.15 KB 43.17 KB RN_FB_PROD
ReactNativeRenderer-profiling.js 0.0% 0.0% 252.67 KB 252.73 KB 44.47 KB 44.49 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.1% +0.1% 699.11 KB 699.69 KB 149.75 KB 149.86 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 246.66 KB 246.72 KB 43.15 KB 43.17 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js 0.0% 0.0% 252.69 KB 252.75 KB 44.47 KB 44.49 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.1% +0.1% 688.04 KB 688.62 KB 147.15 KB 147.25 KB RN_FB_DEV
ReactFabric-prod.js 0.0% 0.0% 239.96 KB 240.02 KB 41.89 KB 41.91 KB RN_FB_PROD
ReactFabric-profiling.js 0.0% 0.0% 245.27 KB 245.33 KB 43.24 KB 43.25 KB RN_FB_PROFILING
ReactFabric-dev.js +0.1% +0.1% 687.95 KB 688.53 KB 147.11 KB 147.21 KB RN_OSS_DEV
ReactFabric-prod.js 0.0% 0.0% 239.97 KB 240.03 KB 41.89 KB 41.91 KB RN_OSS_PROD
ReactFabric-profiling.js 0.0% 0.0% 245.28 KB 245.34 KB 43.23 KB 43.25 KB RN_OSS_PROFILING

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.1% +0.1% 581.26 KB 581.83 KB 123.05 KB 123.16 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.1% 63.51 KB 63.56 KB 19.42 KB 19.43 KB UMD_PROD
react-test-renderer.development.js +0.1% +0.1% 576.88 KB 577.45 KB 121.86 KB 121.96 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.1% 63.21 KB 63.25 KB 19.24 KB 19.25 KB NODE_PROD
ReactTestRenderer-dev.js +0.1% +0.1% 589.56 KB 590.14 KB 122.16 KB 122.28 KB FB_WWW_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.0% 11.7 KB 11.7 KB 3.59 KB 3.59 KB UMD_PROD
react-test-renderer-shallow.development.js 0.0% -0.0% 34.08 KB 34.08 KB 8.58 KB 8.58 KB NODE_DEV
react-test-renderer-shallow.production.min.js 0.0% -0.0% 11.89 KB 11.89 KB 3.7 KB 3.7 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% +0.1% 570.22 KB 570.83 KB 119.64 KB 119.75 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.1% 🔺+0.1% 63.17 KB 63.24 KB 18.77 KB 18.8 KB NODE_PROD
react-reconciler-persistent.development.js +0.1% +0.1% 568.11 KB 568.71 KB 118.78 KB 118.89 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.1% 🔺+0.1% 63.18 KB 63.25 KB 18.78 KB 18.81 KB NODE_PROD
react-reconciler-reflection.development.js 0.0% 0.0% 16.66 KB 16.66 KB 5.07 KB 5.07 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% -0.1% 2.37 KB 2.37 KB 1.07 KB 1.07 KB NODE_PROD

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),
);

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Mar 28, 2019

Member

Duplicate.

@@ -1389,6 +1390,10 @@ function updateSuspenseComponent(
const mode = workInProgress.mode;
const nextProps = workInProgress.pendingProps;

if (shouldSuspend(workInProgress)) {

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Mar 28, 2019

Member

Should be wrapped in __DEV__.

shouldSuspendFiber = catchErrors(fiber =>
hook.shouldSuspendFiber(rendererID, fiber),
);
}

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Mar 28, 2019

Member

Should be wrapped in __DEV__ too.

@acdlite

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Make sure you test in both sync mode and concurrent mode

@gaearon gaearon force-pushed the gaearon:suspense-devtools branch 3 times, most recently from 578f3f0 to da14cfb Mar 29, 2019

@gaearon gaearon changed the title [WIP] Allow DevTools to toggle Suspense fallbacks Allow DevTools to toggle Suspense fallbacks Mar 29, 2019

@gaearon gaearon marked this pull request as ready for review Mar 29, 2019

@gaearon gaearon referenced this pull request Mar 29, 2019

@gaearon gaearon force-pushed the gaearon:suspense-devtools branch from da14cfb to 74265ce Mar 29, 2019

@gaearon

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

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. suspendFiber(true|false) that works like overrideProps in that it both triggers a re-render and could be used as our signal for whether the build of React supports this action?

@gaearon

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

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.

Yeah.

Looking at the implementation, I'm curious: why not inject a method e.g. suspendFiber(true|false) that works like overrideProps in that it both triggers a re-render and could be used as our signal for whether the build of React supports this action?

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.

@sebmarkbage

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

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.

@gaearon

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

During an initial render you can have this pause boundaries that didn’t exist yet.

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;

This comment has been minimized.

Copy link
@acdlite

acdlite Mar 29, 2019

Member

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.

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

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 gaearon force-pushed the gaearon:suspense-devtools branch from 74265ce to 386917c Apr 1, 2019

gaearon added some commits Mar 28, 2019

Change API to overrideSuspense
This lets detect support for overriding Suspense from DevTools.

@gaearon gaearon force-pushed the gaearon:suspense-devtools branch from 386917c to efebb67 Apr 3, 2019

gaearon added some commits Apr 3, 2019

@gaearon

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

I updated the PR to a different API. Now it exposes overrideSuspense instead of asking the Hook to implement shouldSuspend. Calling overrideSuspense is how you supply a custom implementation.

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 overrideSuspense at the Global Hook level during the synchronous injection.

I've also added a CM test.

@gaearon gaearon requested a review from bvaughn Apr 3, 2019

@acdlite

acdlite approved these changes Apr 3, 2019

Copy link
Member

left a comment

Approving the Suspense part. I'll defer to someone else re: the DevTools part.

@bvaughn

bvaughn approved these changes Apr 3, 2019

Copy link
Contributor

left a comment

I think what's here looks fine.

Still not super thrilled about the force re-render bit in the DevTools PR that accompanies this, but I can get over it 😄

@gaearon

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

Still not super thrilled about the force re-render bit in the DevTools PR that accompanies this, but I can get over it 😄

After conversation with Sebastian, I plan to make a few more changes:

  • Rename overrideSuspense to setSuspenseHandler
  • Add scheduleUpdate that removes the need for overrideProps hack

In the future, we'll move away from the overrideProps mechanism to (out of scope for this PR) setPropsHandler which would let us delegate the overriding logic to DevTools itself. That would let us implement overrides as shadowing instead of the current fragile mechanism.

@bvaughn

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Nice nice! I like both of those changes 👍

In the future, we'll move away from the overrideProps mechanism to (out of scope for this PR) setPropsHandler which would let us delegate the overriding logic to DevTools itself. That would let us implement overrides as shadowing instead of the current fragile mechanism.

Interesting. I'd love to talk more about this. I'm a little wary of the shadowing idea. (We've talked about this a bit in the past, either with you or Sebastian I forget.)

@gaearon gaearon merged commit 7a2dc48 into facebook:master Apr 4, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@gaearon gaearon deleted the gaearon:suspense-devtools branch Apr 4, 2019

Kiku-Reise added a commit to Kiku-Reise/react that referenced this pull request May 16, 2019

Allow DevTools to toggle Suspense fallbacks (facebook#15232)
* Allow DevTools to toggle Suspense state

* Change API to overrideSuspense

This lets detect support for overriding Suspense from DevTools.

* Add ConcurrentMode test

* Newlines

* Remove unnecessary change

* Naming changes
@gaearon gaearon referenced this pull request Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.