[refactor] Add element type for Activity #32499
Conversation
github-actions
bot
added
the
React Core Team
| mode, | ||
| renderLanes, | ||
| ); | ||
| primaryChildFragment.ref = workInProgress.ref; |
There was a problem hiding this comment.
@sebmarkbage currently I'm just forwarding the ref to the Offscreen element child, and not changing how manual mode works. As a follow up, should we make it so the ref is only exposed to the Activity element, which finds the Offscreen child to toggle visibility?
There was a problem hiding this comment.
Removed manual mode in #32645
rickhanlonii
force-pushed
the
rh/activity-element
branch
from
0e629b4 to
b045f18
Compare
| key: null | string, | ||
| ): Fiber { | ||
| const fiber = createFiber(ActivityComponent, pendingProps, key, mode); | ||
| fiber.elementType = REACT_ACTIVITY_TYPE; |
There was a problem hiding this comment.
This is not actually right. This was wrong for Offscreen too.
In the case of const LazyActivity = React.lazy(async () => ({ default: React.Activity })); <LazyActivity /> the elementType should be the Lazy wrapper object. Otherwise reconciliation won't work correctly.
In fact, it looks like we get this wrong for all these built-ins so wrapping them would yield the wrong reconciliation.
So it's an existing bug that we need to fix separately.
There was a problem hiding this comment.
Is this wrong for REACT_VIEW_TRANSITION_TYPE too?
What's the fix?
There was a problem hiding this comment.
Actually, we throw for this case already with an error that says:
Caution
Element type is invalid. Received a promise that resolves to: ViewTransition. Lazy element type must resolve to a class or function.
This is broken for portal, so I fixed it and tested all the built-ins here: #32640
Just adding the name so it shows up. (Note that no experimental ones are added to the list of filters atm. Including SuspenseList etc.)
rickhanlonii
force-pushed
the
rh/activity-element
branch
from
b045f18 to
86503fe
Compare
packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js
Outdated
Show resolved
Hide resolved
rickhanlonii
force-pushed
the
rh/activity-element
branch
from
265d395 to
9aadf4b
Compare
rickhanlonii
force-pushed
the
rh/activity-element
branch
from
9aadf4b to
2f230f0
Compare
|
Pushed some fixes for owner stacks and devtools. I decided to include those here so we can reference one PR with the changes needed to add a new element, instead of splitting it out. In followups I'll: |
rickhanlonii
force-pushed
the
rh/activity-element
branch
from
c67bb5a to
cdfb5f6
Compare
Based off: #32499 While looking into `React.lazy` issues for built-ins, I noticed we already error for `lazy` with build-ins, but we don't have any tests for `getComponentNameFromType` using all the built-ins. This may be something we should handle, but for now we should at least have tests. Here's why: while writing tests, I noticed we check `type` instead of `$$typeof` for portals: https://github.com/facebook/react/blob/9cdf8a99edcfd94d7420835ea663edca04237527/packages/react-reconciler/src/ReactPortal.js#L25-L32 This PR adds tests for all the built-ins and fixes the portal bug. [Commit to review](e068c16)
Based off: #32499 While looking into `React.lazy` issues for built-ins, I noticed we already error for `lazy` with build-ins, but we don't have any tests for `getComponentNameFromType` using all the built-ins. This may be something we should handle, but for now we should at least have tests. Here's why: while writing tests, I noticed we check `type` instead of `$$typeof` for portals: https://github.com/facebook/react/blob/9cdf8a99edcfd94d7420835ea663edca04237527/packages/react-reconciler/src/ReactPortal.js#L25-L32 This PR adds tests for all the built-ins and fixes the portal bug. [Commit to review](e068c16) DiffTrain build for [8243f3f](8243f3f)
Based off: #32499 While looking into `React.lazy` issues for built-ins, I noticed we already error for `lazy` with build-ins, but we don't have any tests for `getComponentNameFromType` using all the built-ins. This may be something we should handle, but for now we should at least have tests. Here's why: while writing tests, I noticed we check `type` instead of `$$typeof` for portals: https://github.com/facebook/react/blob/9cdf8a99edcfd94d7420835ea663edca04237527/packages/react-reconciler/src/ReactPortal.js#L25-L32 This PR adds tests for all the built-ins and fixes the portal bug. [Commit to review](e068c16) DiffTrain build for [8243f3f](8243f3f)
Followup from #32499 Manual mode is unused and has some bugs such as revealing hidden boundaries when manually toggling. We also want to change how manual mode works, and do some refactors to Activity to make it easier to support. For now we'll remove it, then add it back after the other changes we have planned.
This PR separates Activity to it's own element type separate from Offscreen. The goal is to allow us to add Activity element boundary semantics during hydration similar to Suspense semantics, without impacting the Offscreen behavior in suspended children.