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

Jump to bottom

Fix use of stale props in Fabric events #26408

Merged
merged 2 commits into from
Mar 17, 2023
Merged

Fix use of stale props in Fabric events #26408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f039d8
Revert "Extracted definition and access to public instances to a sepa…
rubennorte Mar 16, 2023
7f86a0c
Extracted definition and access to public instances to a separate mod…
rubennorte Mar 16, 2023
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: 6 additions & 3 deletions packages/react-native-renderer/src/ReactFabricComponentTree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,11 @@ import {getPublicInstance} from './ReactFabricHostConfig';
function getInstanceFromNode(node: Instance | TextInstance): Fiber | null {
const instance: Instance = (node: $FlowFixMe); // In React Native, node is never a text instance

if (instance.internalInstanceHandle != null) {
return instance.internalInstanceHandle;
if (
instance.canonical != null &&
instance.canonical.internalInstanceHandle != null
) {
return instance.canonical.internalInstanceHandle;
}

// $FlowFixMe[incompatible-return] DevTools incorrectly passes a fiber in React Native.
Expand All @@ -41,7 +44,7 @@ function getNodeFromInstance(fiber: Fiber): PublicInstance {
}

function getFiberCurrentPropsFromNode(instance: Instance): Props {
return instance.currentProps;
return instance.canonical.currentProps;
}

export {
Expand Down
50 changes: 23 additions & 27 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,15 @@ export type Props = Object;
export type Instance = {
// Reference to the shadow node.
node: Node,
nativeTag: number,
viewConfig: ViewConfig,
currentProps: Props,
// Reference to the React handle (the fiber)
internalInstanceHandle: Object,
// Exposed through refs.
publicInstance: ReactFabricHostComponent,
canonical: {
nativeTag: number,
viewConfig: ViewConfig,
currentProps: Props,
// Reference to the React handle (the fiber)
internalInstanceHandle: Object,
// Exposed through refs.
publicInstance: ReactFabricHostComponent,
},
};
export type TextInstance = {node: Node, ...};
export type HydratableInstance = Instance | TextInstance;
Expand Down Expand Up @@ -148,11 +150,13 @@ export function createInstance(

return {
node: node,
nativeTag: tag,
viewConfig,
currentProps: props,
internalInstanceHandle,
publicInstance: component,
canonical: {
nativeTag: tag,
viewConfig,
currentProps: props,
internalInstanceHandle,
publicInstance: component,
},
};
}

Expand Down Expand Up @@ -222,8 +226,8 @@ export function getChildHostContext(
}

export function getPublicInstance(instance: Instance): null | PublicInstance {
if (instance.publicInstance != null) {
return instance.publicInstance;
if (instance.canonical != null && instance.canonical.publicInstance != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a null-check here? The types claim these are all non-nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember why I did the original check (should've added a comment) but there's some code like this for compatibility with Paper, so types don't match 1:1. I'll double check.

return instance.canonical.publicInstance;
}

// For compatibility with the legacy renderer, in case it's used with Fabric
Expand All @@ -249,12 +253,12 @@ export function prepareUpdate(
newProps: Props,
hostContext: HostContext,
): null | Object {
const viewConfig = instance.viewConfig;
const viewConfig = instance.canonical.viewConfig;
const updatePayload = diff(oldProps, newProps, viewConfig.validAttributes);
// TODO: If the event handlers have changed, we need to update the current props
// in the commit phase but there is no host config hook to do it yet.
// So instead we hack it by updating it in the render phase.
instance.currentProps = newProps;
instance.canonical.currentProps = newProps;
return updatePayload;
}

Expand Down Expand Up @@ -333,11 +337,7 @@ export function cloneInstance(
}
return {
node: clone,
nativeTag: instance.nativeTag,
viewConfig: instance.viewConfig,
currentProps: instance.currentProps,
internalInstanceHandle: instance.internalInstanceHandle,
publicInstance: instance.publicInstance,
canonical: instance.canonical,
};
}

Expand All @@ -347,19 +347,15 @@ export function cloneHiddenInstance(
props: Props,
internalInstanceHandle: Object,
): Instance {
const viewConfig = instance.viewConfig;
const viewConfig = instance.canonical.viewConfig;
const node = instance.node;
const updatePayload = create(
{style: {display: 'none'}},
viewConfig.validAttributes,
);
return {
node: cloneNodeWithNewProps(node, updatePayload),
nativeTag: instance.nativeTag,
viewConfig: instance.viewConfig,
currentProps: instance.currentProps,
internalInstanceHandle: instance.internalInstanceHandle,
publicInstance: instance.publicInstance,
canonical: instance.canonical,
};
}

Expand Down
6 changes: 3 additions & 3 deletions packages/react-native-renderer/src/ReactNativeComponentTree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ function getInstanceFromTag(tag) {
function getTagFromInstance(inst) {
let nativeInstance = inst.stateNode;
let tag = nativeInstance._nativeTag;
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename these methods? It's already exported as getNodeFromInstance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still need to do some more work on the react repo for this. Can do that in a following PR.

if (tag === undefined) {
if (tag === undefined && nativeInstance.canonical != null) {
// For compatibility with Fabric
tag = nativeInstance.nativeTag;
nativeInstance = nativeInstance.publicInstance;
tag = nativeInstance.canonical.nativeTag;
nativeInstance = nativeInstance.canonical.publicInstance;
}

if (!tag) {
Expand Down
5 changes: 3 additions & 2 deletions packages/react-native-renderer/src/ReactNativeFiberInspector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,10 +223,11 @@ function getInspectorDataForViewAtPoint(
}

closestInstance =
Copy link
Member

Choose a reason for hiding this comment

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

This was previously using a private property on ReactFabricHostComponent, right? This seems to be the only place we need to access internalInstanceHandle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was previously using a private property on ReactFabricHostComponent, right?

Yeah.

This seems to be the only place we need to access internalInstanceHandle.

There's another one in ReactFabricComponentTree. What are you suggesting?

Copy link
Member

Choose a reason for hiding this comment

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

Just trying to see if we can avoid the prop and re-use the existing one, but since this is now on canonical, it probably doesn't matter as much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could use symbol properties on the public instance to avoid creating new properties and keep encapsulation, but I want to make public instances lazy and that would prevent me from doing it.

internalInstanceHandle.stateNode.internalInstanceHandle;
internalInstanceHandle.stateNode.canonical.internalInstanceHandle;

// Note: this is deprecated and we want to remove it ASAP. Keeping it here for React DevTools compatibility for now.
const nativeViewTag = internalInstanceHandle.stateNode.nativeTag;
const nativeViewTag =
internalInstanceHandle.stateNode.canonical.nativeTag;

nativeFabricUIManager.measure(
node,
Expand Down
4 changes: 2 additions & 2 deletions packages/react-native-renderer/src/ReactNativeHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,8 @@ export function getChildHostContext(

export function getPublicInstance(instance: Instance): * {
// $FlowExpectedError[prop-missing] For compatibility with Fabric
if (instance.publicInstance != null) {
return instance.publicInstance;
if (instance.canonical != null && instance.canonical.publicInstance != null) {
return instance.canonical.publicInstance;
}

return instance;
Expand Down
14 changes: 10 additions & 4 deletions packages/react-native-renderer/src/ReactNativePublicCompat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,12 @@ export function findHostInstance_DEPRECATED<TElementType: ElementType>(
}

// For compatibility with Fabric instances
if (componentOrHandle.publicInstance) {
if (
componentOrHandle.canonical &&
componentOrHandle.canonical.publicInstance
) {
// $FlowExpectedError[incompatible-return] Can't refine componentOrHandle as a Fabric instance
return componentOrHandle.publicInstance;
return componentOrHandle.canonical.publicInstance;
}

// For compatibility with legacy renderer instances
Expand Down Expand Up @@ -117,8 +120,11 @@ export function findNodeHandle(componentOrHandle: any): ?number {
}

// For compatibility with Fabric instances
if (componentOrHandle.nativeTag != null) {
return componentOrHandle.nativeTag;
if (
componentOrHandle.canonical != null &&
componentOrHandle.canonical.nativeTag != null
) {
return componentOrHandle.canonical.nativeTag;
}

// For compatibility with Fabric public instances
Expand Down