-
Notifications
You must be signed in to change notification settings - Fork 55
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
[MVP] Toggle suspense #36
Conversation
Haven't looked at the code yet, but the demo looks slick!
Yeah, or maybe we could show a custom (fake) boolean prop e.g.
That's fair. We'll need to strike a balance that accounts for discoverability and ease-of-access, given limited space.
Maybe we could color the
I assume you mean within the "Elements" panel? Hm. The tree-to-flat-list logic is kind of complicated, and pretty performance sensitive. I'd kind of prefer not to monkey with it too much unless necessary. Maybe we could do something like dim descendants or something though.
Hm...what's the use case here? Why are people using this feature? My assumption: Just to visually test how their app looks in various stages of loading. I'm not convinced that "untoggle all" is a necessary interaction for this use case.
Ah, yes. I think maybe so. |
I was excited to try this out on the suspense boundaries in DevTools itself, but I forgot to make a build of React from facebook/react#15232 and it crashed. We'll need to figure out a feature detection strategy for this. |
// TODO: this will incorrectly enable it on old versions. | ||
canToggleSuspense: | ||
typeof overrideProps === 'function' && | ||
tag === ReactTypeOfWork.SuspenseComponent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice TODO. This should probably look for a new injected flag that signals whether the current version and build supports the override behavior.
} | ||
// Force a re-render. | ||
const fiber = idToFiberMap.get(id); | ||
overrideProps(fiber, [], null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious: why not inject a method like e.g. suspendFiber(true)
that could be used as our signal for support as well as our method for re-rendering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to preserve our ability to do filtering based on something other than just putting it in a Set.
Built the profiler demo page with branch of React so I could test this on the profiler itself: Pretty slick! It actually helped me realize that the UI for that first fallback isn't great, and I was able to iterate on it in the Chrome Elements CSS tools and make it better (a450a23) 😄 |
@@ -60,6 +61,8 @@ export default function ButtonIcon({ type }: Props) { | |||
case 'search': | |||
pathData = PATH_SEARCH; | |||
break; | |||
case 'toggle-suspense': | |||
return '🌀'; // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆 You love this emoji
> | ||
<ButtonIcon type="toggle-suspense" /> | ||
</Button> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wherever we end up placing this toggle, we should also:
- Show it as a toggle (with a clear on/off state).
- Only show it if the
Suspense
boundary has actually resolved. (Updates that trigger new suspends might be a little tricky, but maybe we can just ignore that.)
#61 addresses the concerns. |
To be used with facebook/react#15232.
Adds a toolbar button that toggles Suspense boundary state.
Some open questions:
Out of scope: