Allow to toggle Suspense in Components pane #61
Conversation
|
I pushed another commit to fix #60. Without it, it's kind of a pain to inspect large trees.
|
bvaughn
left a comment
bvaughn
left a comment
There was a problem hiding this comment.
Looks good! Couple of minor nits requested, but otherwise LGTM~
shells/dev/app/SuspenseTree/index.js
Outdated
| function LoadLater() { | ||
| const [loadChild, setLoadChild] = useState(0); | ||
| return ( | ||
| <Suspense |
There was a problem hiding this comment.
Selecting this element in the DevTools tree and clicking the load/reload button causes an error:
Uncaught TypeError: Cannot read property 'parentID' of undefined
at _loop (devtools.js:13578)
at Bridge.<anonymous> (devtools.js:13649)
at Bridge.emit (devtools.js:1621)
at devtools.js:12785
at devtools.js:21985
There was a problem hiding this comment.
Actually this happens when you toggle via the inspector too (makes sense). Seems like a more important thing to fix.
src/backend/renderer.js
Outdated
| // Does the current renderer support editable function props? | ||
| canEditFunctionProps: typeof overrideProps === 'function', | ||
|
|
||
| canEditSuspense: |
src/backend/renderer.js
Outdated
| setSuspenseHandler, | ||
| scheduleUpdate, | ||
| } = renderer; | ||
| const supportsEditingSuspense = |
There was a problem hiding this comment.
naming nit supportsTogglingSuspense ?
| function overrideSuspense(id, forceFallback) { | ||
| if ( | ||
| typeof setSuspenseHandler !== 'function' || | ||
| typeof scheduleUpdate !== 'function' |
There was a problem hiding this comment.
nit could we just check !supportsEditingSuspense here?
There was a problem hiding this comment.
Flow wouldn’t be happy
shells/dev/app/index.js
Outdated
| mountHelper(ElementTypes); | ||
| mountHelper(EditableProps); | ||
| mountHelper(DeeplyNestedComponents); | ||
| mountHelper(SuspenseTree); |
There was a problem hiding this comment.
nit Move this above DeeplyNestedComponents so we can test it without as much scrolling 😆
There was a problem hiding this comment.
I figured maybe you test some other case more often. But fair re: DeeplyNested ones. :-)
| } | ||
| } | ||
| const fiber = idToFiberMap.get(id); | ||
| scheduleUpdate(fiber); |
There was a problem hiding this comment.
Do we need to call this with the current Fiber or is it robust enough to take either one?
There was a problem hiding this comment.
Either one would work.
There was a problem hiding this comment.
That makes sense. Just wanted to confirm.
| data={state} | ||
| overrideValueFn={overrideStateFn} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
I hope we don't end up needing more branching like this 😅
There was a problem hiding this comment.
Can refactor if we do. :-) I haven’t seen other cases but they might come up if we add memoizedState to other types.
gaearon
force-pushed
the
toggle-suspense-2
branch
from
9f9e0b5 to
90eb2c2
Compare
gaearon
force-pushed
the
toggle-suspense-2
branch
from
90eb2c2 to
f107189
Compare
|
This looks good to me. Shall we merge it? 😄 |
|
Still need to fix the suspense fragment behavior. It doesn’t always show the right thing in the tree. Not fault of this PR but I’d like to fix them together. |
|
I'll get this in, but I'll keep working on the actual fix for Suspense fragment stuff separately. Because the problem already exists in master anyway. |
gaearon
force-pushed
the
toggle-suspense-2
branch
from
f107189 to
b8245de
Compare
| // Try our best to find the fallback directly. | ||
| const maybeFallbackFiber = | ||
| (fiber.child && fiber.child.sibling) || fiber; | ||
| return renderer.findHostInstanceByFiber(maybeFallbackFiber); |
There was a problem hiding this comment.
Should this logic to skip a timed-out Suspense's primary children live in React itself, in findCurrentHostFiber? I don't think it makes a lot of sense for this to live in devtools.
If you findDOMNode from outside a Suspense, you shouldn't get a node back from the primary children. (I think?)
cc @acdlite
3 participants
Supersedes #36.
Fixes #60.
This is MVP support for toggling Suspense from the Components pane. We plan a separate tab in #43 but we'd want a way to quickly debug it anyway so seems like this is a good first step.
Showing Suspense Status
Currently, Suspense shows Fiber internal state in the "state" section of the inspector (when Suspended), and nothing if unsuspended. This PR changes so that we show the current Suspense state:
This works with older versions of React.
Toggling Suspense Status
The inspected value turns into a checkbox if:
This means that you can toggle Suspense on and off. But if a component actually Suspends by itself, there's nothing to show in the "current" state. In that case the checkbox becomes a read-only label again until it un-Suspends.
Alternatively, we could separate the "current value" from the "override value". But I feel like this is sufficient and intuitive enough in practice after playing with it.
Implementation
The simplest implementation would be to always
setSuspenseHandler()to the thing that checks IDs in the Set. However, I don't want to incur any Set checking overhead if none of the Suspenses were manually toggled. So I attach and detach the Set-checking handler on demand.