[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?
|
Comparing: 2980f27...84f0996 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
| @@ -2261,6 +2262,7 @@ function renderElement( | |||
| task.keyPath = prevKeyPath; | |||
| return; | |||
| } | |||
| case REACT_ACTIVITY_TYPE: | |||
There was a problem hiding this comment.
@sebmarkbage this might not be correct - to match the client it should render an Offscreen element, but I'm not sure how to do that in Fizz. Without that, I think this might cause a hydration error if it's rendered in mode visible?
There was a problem hiding this comment.
Offscreen is an implementation detail of Fiber and it will never be noticeable in the HTML so it can't hydrate.
Basically Fizz doesn't need to know about the concept of Offscreen. It doesn't need the same type of code sharing between Suspense and Activity that Fiber needs to manage the complexity. So it can be just forked paths.
In other words, you can just drop REACT_OFFSCREEN_TYPE here and rename it to Activity.
| @@ -867,6 +868,46 @@ function deferHiddenOffscreenComponent( | |||
| // fork the function. | |||
| const updateLegacyHiddenComponent = updateOffscreenComponent; | |||
|
|
|||
| function updateActivityComponent( | |||
There was a problem hiding this comment.
Needs a close review, I have no idea what I'm doing. mountWorkInProgressOffscreenFiber and updateWorkInProgressOffscreenFiber are what Suspense use, so I assume that's how we want to mount and update the children here?
There was a problem hiding this comment.
Yea this sounds about right.
rickhanlonii
force-pushed
the
rh/activity-element
branch
from
0e629b4 to
b045f18
Compare
| @@ -34,6 +34,7 @@ export const REACT_MEMO_TYPE: symbol = Symbol.for('react.memo'); | |||
| export const REACT_LAZY_TYPE: symbol = Symbol.for('react.lazy'); | |||
| export const REACT_SCOPE_TYPE: symbol = Symbol.for('react.scope'); | |||
| export const REACT_OFFSCREEN_TYPE: symbol = Symbol.for('react.offscreen'); | |||
There was a problem hiding this comment.
Is this still used anywhere or can this be deleted now?
There was a problem hiding this comment.
Basically the idea is that React internally only refers to Offscreen using the tag. It should never have to look at .type or .elementType. The createFiberFromOffscreen should exclude it since it's not needed for the elementType. It can be null. elementType is only used for reconciliation against new elements but since Offscreen is only created internally it should never need to be reconciled. And so it should be impossible to create this symbol an all the cases that try to match it.
| @@ -1480,6 +1488,7 @@ export function attach( | |||
| return true; | |||
| case HostPortal: | |||
| case HostText: | |||
| case ActivityComponent: | |||
There was a problem hiding this comment.
So this is actually a good example for why we need this to be a separate wrapper. Because this should not be ignored. It should be visible in React DevTools because it's not an internal implementation detail but actually something the user renders.
The reason OffscreenComponent was hidden was because it's an internal of Suspense and it would be very noisy to show it everywhere but the Activity one we actually want to show.
So this should actually go into one of the other paths (and be filterable as a "type" in the filters).
This could be done as a follow up though.
| @@ -595,6 +597,8 @@ export function createFiberFromTypeAndProps( | |||
| } | |||
| } else { | |||
| getTag: switch (type) { | |||
| case REACT_ACTIVITY_TYPE: | |||
There was a problem hiding this comment.
Seems like we should be able to delete the Offscreen case?
| 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.
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.