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.

Sign up for GitHub

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

[refactor] Add element type for Activity #32499

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+84 −3
Open

[refactor] Add element type for Activity #32499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/react-devtools-shared/src/backend/fiber/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ import type {
import type {Source} from 'react-devtools-shared/src/shared/types';
import {getSourceLocationByFiber} from './DevToolsFiberComponentStack';
import {formatOwnerStack} from '../shared/DevToolsOwnerStack';
import {ActivityComponent} from 'react-reconciler/src/ReactWorkTags';

// Kinds
const FIBER_INSTANCE = 0;
Expand Down Expand Up @@ -385,6 +386,7 @@ export function getInternalReactConstants(version: string): {
YieldComponent: -1, // Removed
Throw: 29,
ViewTransitionComponent: 30, // Experimental
ActivityComponent: 31,
};
} else if (gte(version, '17.0.0-alpha')) {
ReactTypeOfWork = {
Expand Down Expand Up @@ -421,6 +423,7 @@ export function getInternalReactConstants(version: string): {
YieldComponent: -1, // Removed
Throw: -1, // Doesn't exist yet
ViewTransitionComponent: -1, // Doesn't exist yet
ActivityComponent: -1, // Doesn't exist yet
};
} else if (gte(version, '16.6.0-beta.0')) {
ReactTypeOfWork = {
Expand Down Expand Up @@ -457,6 +460,7 @@ export function getInternalReactConstants(version: string): {
YieldComponent: -1, // Removed
Throw: -1, // Doesn't exist yet
ViewTransitionComponent: -1, // Doesn't exist yet
ActivityComponent: -1, // Doesn't exist yet
};
} else if (gte(version, '16.4.3-alpha')) {
ReactTypeOfWork = {
Expand Down Expand Up @@ -493,6 +497,7 @@ export function getInternalReactConstants(version: string): {
YieldComponent: -1, // Removed
Throw: -1, // Doesn't exist yet
ViewTransitionComponent: -1, // Doesn't exist yet
ActivityComponent: -1, // Doesn't exist yet
};
} else {
ReactTypeOfWork = {
Expand Down Expand Up @@ -529,6 +534,7 @@ export function getInternalReactConstants(version: string): {
YieldComponent: 9,
Throw: -1, // Doesn't exist yet
ViewTransitionComponent: -1, // Doesn't exist yet
ActivityComponent: -1, // Doesn't exist yet
};
}
// **********************************************************
Expand Down Expand Up @@ -622,6 +628,8 @@ export function getInternalReactConstants(version: string): {
}

switch (tag) {
case ActivityComponent:
return 'Activity';
case CacheComponent:
return 'Cache';
case ClassComponent:
Expand Down Expand Up @@ -1480,6 +1488,7 @@ export function attach(
return true;
case HostPortal:
case HostText:
case ActivityComponent:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case LegacyHiddenComponent:
case OffscreenComponent:
case Throw:
Expand Down
1 change: 1 addition & 0 deletions packages/react-devtools-shared/src/backend/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export type WorkTagMap = {
YieldComponent: WorkTag,
Throw: WorkTag,
ViewTransitionComponent: WorkTag,
ActivityComponent: WorkTag,
};

export type HostInstance = Object;
Expand Down
15 changes: 15 additions & 0 deletions packages/react-reconciler/src/ReactFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ import {
TracingMarkerComponent,
Throw,
ViewTransitionComponent,
ActivityComponent,
} from './ReactWorkTags';
import {OffscreenVisible} from './ReactFiberActivityComponent';
import {getComponentNameFromOwner} from 'react-reconciler/src/getComponentNameFromFiber';
Expand Down Expand Up @@ -108,6 +109,7 @@ import {
REACT_TRACING_MARKER_TYPE,
REACT_ELEMENT_TYPE,
REACT_VIEW_TRANSITION_TYPE,
REACT_ACTIVITY_TYPE,
} from 'shared/ReactSymbols';
import {TransitionTracingMarker} from './ReactFiberTracingMarkerComponent';
import {
Expand Down Expand Up @@ -595,6 +597,8 @@ export function createFiberFromTypeAndProps(
}
} else {
getTag: switch (type) {
case REACT_ACTIVITY_TYPE:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we should be able to delete the Offscreen case?

return createFiberFromActivity(pendingProps, mode, lanes, key);
case REACT_FRAGMENT_TYPE:
return createFiberFromFragment(pendingProps.children, mode, lanes, key);
case REACT_STRICT_MODE_TYPE:
Expand Down Expand Up @@ -874,6 +878,17 @@ export function createFiberFromOffscreen(
fiber.stateNode = primaryChildInstance;
return fiber;
}
export function createFiberFromActivity(
pendingProps: OffscreenProps,
mode: TypeOfMode,
lanes: Lanes,
key: null | string,
): Fiber {
const fiber = createFiber(ActivityComponent, pendingProps, key, mode);
fiber.elementType = REACT_ACTIVITY_TYPE;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fiber.lanes = lanes;
return fiber;
}

export function createFiberFromViewTransition(
pendingProps: ViewTransitionProps,
Expand Down
44 changes: 44 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ import {
TracingMarkerComponent,
Throw,
ViewTransitionComponent,
ActivityComponent,
} from './ReactWorkTags';
import {
NoFlags,
Expand Down Expand Up @@ -867,6 +868,46 @@ function deferHiddenOffscreenComponent(
// fork the function.
const updateLegacyHiddenComponent = updateOffscreenComponent;

function updateActivityComponent(
Copy link
Member Author

@rickhanlonii rickhanlonii Mar 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea this sounds about right.

current: null | Fiber,
workInProgress: Fiber,
renderLanes: Lanes,
) {
const nextProps = workInProgress.pendingProps;
const nextChildren = nextProps.children;
const nextMode = nextProps.mode;
const mode = workInProgress.mode;
const offscreenChildProps: OffscreenProps = {
mode: nextMode,
children: nextChildren,
};

if (current === null) {
const primaryChildFragment = mountWorkInProgressOffscreenFiber(
offscreenChildProps,
mode,
renderLanes,
);
primaryChildFragment.ref = workInProgress.ref;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

workInProgress.child = primaryChildFragment;
primaryChildFragment.return = workInProgress;

return primaryChildFragment;
} else {
const currentChild: Fiber = (current.child: any);

const primaryChildFragment = updateWorkInProgressOffscreenFiber(
currentChild,
offscreenChildProps,
);

primaryChildFragment.ref = workInProgress.ref;
workInProgress.child = primaryChildFragment;
primaryChildFragment.return = workInProgress;
return primaryChildFragment;
}
}

function updateCacheComponent(
current: Fiber | null,
workInProgress: Fiber,
Expand Down Expand Up @@ -4024,6 +4065,9 @@ function beginWork(
}
break;
}
case ActivityComponent: {
return updateActivityComponent(current, workInProgress, renderLanes);
}
case OffscreenComponent: {
return updateOffscreenComponent(current, workInProgress, renderLanes);
}
Expand Down
2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ import {
TracingMarkerComponent,
Throw,
ViewTransitionComponent,
ActivityComponent,
} from './ReactWorkTags';
import {NoMode, ConcurrentMode, ProfileMode} from './ReactTypeOfMode';
import {
Expand Down Expand Up @@ -959,6 +960,7 @@ function completeWork(
}
// Fallthrough
}
case ActivityComponent:
case LazyComponent:
case SimpleMemoComponent:
case FunctionComponent:
Expand Down
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/ReactWorkTags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ export type WorkTag =
| 27
| 28
| 29
| 30;
| 30
| 31;

export const FunctionComponent = 0;
export const ClassComponent = 1;
Expand Down Expand Up @@ -69,3 +70,4 @@ export const HostSingleton = 27;
export const IncompleteFunctionComponent = 28;
export const Throw = 29;
export const ViewTransitionComponent = 30;
export const ActivityComponent = 31;
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/getComponentNameFromFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import {
TracingMarkerComponent,
Throw,
ViewTransitionComponent,
ActivityComponent,
} from 'react-reconciler/src/ReactWorkTags';
import getComponentNameFromType from 'shared/getComponentNameFromType';
import {REACT_STRICT_MODE_TYPE} from 'shared/ReactSymbols';
Expand Down Expand Up @@ -85,6 +86,8 @@ export function getComponentNameFromOwner(
export default function getComponentNameFromFiber(fiber: Fiber): string | null {
const {tag, type} = fiber;
switch (tag) {
case ActivityComponent:
return 'Activity';
case CacheComponent:
return 'Cache';
case ContextConsumer:
Expand Down
2 changes: 2 additions & 0 deletions packages/react-server/src/ReactFizzServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ import {
REACT_OFFSCREEN_TYPE,
REACT_POSTPONE_TYPE,
REACT_VIEW_TRANSITION_TYPE,
REACT_ACTIVITY_TYPE,
} from 'shared/ReactSymbols';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
Expand Down Expand Up @@ -2261,6 +2262,7 @@ function renderElement(
task.keyPath = prevKeyPath;
return;
}
case REACT_ACTIVITY_TYPE:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case REACT_OFFSCREEN_TYPE: {
renderOffscreen(request, task, keyPath, props);
return;
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/ReactClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
REACT_SUSPENSE_TYPE,
REACT_SUSPENSE_LIST_TYPE,
REACT_LEGACY_HIDDEN_TYPE,
REACT_OFFSCREEN_TYPE,
REACT_ACTIVITY_TYPE,
REACT_SCOPE_TYPE,
REACT_TRACING_MARKER_TYPE,
REACT_VIEW_TRANSITION_TYPE,
Expand Down Expand Up @@ -116,7 +116,7 @@ export {
useDeferredValue,
REACT_SUSPENSE_LIST_TYPE as unstable_SuspenseList,
REACT_LEGACY_HIDDEN_TYPE as unstable_LegacyHidden,
REACT_OFFSCREEN_TYPE as unstable_Activity,
REACT_ACTIVITY_TYPE as unstable_Activity,
getCacheForType as unstable_getCacheForType,
useCacheRefresh as unstable_useCacheRefresh,
use,
Expand Down
1 change: 1 addition & 0 deletions packages/shared/ReactSymbols.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still used anywhere or can this be deleted now?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

export const REACT_ACTIVITY_TYPE: symbol = Symbol.for('react.activity');
export const REACT_LEGACY_HIDDEN_TYPE: symbol = Symbol.for(
'react.legacy_hidden',
);
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/isValidElementType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
REACT_OFFSCREEN_TYPE,
REACT_TRACING_MARKER_TYPE,
REACT_VIEW_TRANSITION_TYPE,
REACT_ACTIVITY_TYPE,
} from 'shared/ReactSymbols';
import {
enableScopeAPI,
Expand All @@ -50,6 +51,7 @@ export default function isValidElementType(type: mixed): boolean {
type === REACT_SUSPENSE_TYPE ||
type === REACT_SUSPENSE_LIST_TYPE ||
(enableLegacyHidden && type === REACT_LEGACY_HIDDEN_TYPE) ||
type === REACT_ACTIVITY_TYPE ||
type === REACT_OFFSCREEN_TYPE ||
(enableScopeAPI && type === REACT_SCOPE_TYPE) ||
(enableTransitionTracing && type === REACT_TRACING_MARKER_TYPE) ||
Expand Down
Loading