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 ReactFabricHostComponent methods if detached #21967

Merged
merged 1 commit into from
Jul 27, 2021
Merged

Fix ReactFabricHostComponent methods if detached #21967

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 26 additions & 14 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 @@ -137,17 +137,23 @@ class ReactFabricHostComponent {
}

measure(callback: MeasureOnSuccessCallback) {
fabricMeasure(
this._internalInstanceHandle.stateNode.node,
mountSafeCallback_NOT_REALLY_SAFE(this, callback),
);
const {stateNode} = this._internalInstanceHandle;
if (stateNode != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be silently ignored? Does it indicate a bug somewhere? It seems a lot like the warning we show when you call setState on an unmounted component. What do you think about a warning like "you called a command on an unmounted component"?

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 feel like I should mirror what React does when you update state on an unmounted component. Should I be using console.warn or console.error? And should it only be done in __DEV__?

fabricMeasure(
stateNode.node,
mountSafeCallback_NOT_REALLY_SAFE(this, callback),
);
}
}

measureInWindow(callback: MeasureInWindowOnSuccessCallback) {
fabricMeasureInWindow(
this._internalInstanceHandle.stateNode.node,
mountSafeCallback_NOT_REALLY_SAFE(this, callback),
);
const {stateNode} = this._internalInstanceHandle;
if (stateNode != null) {
fabricMeasureInWindow(
stateNode.node,
mountSafeCallback_NOT_REALLY_SAFE(this, callback),
);
}
}

measureLayout(
Expand All @@ -168,12 +174,18 @@ class ReactFabricHostComponent {
return;
}

fabricMeasureLayout(
this._internalInstanceHandle.stateNode.node,
relativeToNativeNode._internalInstanceHandle.stateNode.node,
mountSafeCallback_NOT_REALLY_SAFE(this, onFail),
mountSafeCallback_NOT_REALLY_SAFE(this, onSuccess),
);
const toStateNode = this._internalInstanceHandle.stateNode;
const fromStateNode =
relativeToNativeNode._internalInstanceHandle.stateNode;

if (toStateNode != null && fromStateNode != null) {
fabricMeasureLayout(
toStateNode.node,
fromStateNode.node,
mountSafeCallback_NOT_REALLY_SAFE(this, onFail),
mountSafeCallback_NOT_REALLY_SAFE(this, onSuccess),
);
}
}

setNativeProps(nativeProps: Object) {
Expand Down
175 changes: 175 additions & 0 deletions packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,37 @@ describe('ReactFabric', () => {
expect(successCallback).toHaveBeenCalledWith(10, 10, 100, 100, 0, 0);
});

it('should no-op if calling measure on unmounted refs', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

nativeFabricUIManager.measure.mockClear();

let viewRef;
act(() => {
ReactFabric.render(
<View
ref={ref => {
viewRef = ref;
}}
/>,
11,
);
});
const dangerouslyRetainedViewRef = viewRef;
act(() => {
ReactFabric.stopSurface(11);
});

expect(nativeFabricUIManager.measure).not.toBeCalled();
const successCallback = jest.fn();
dangerouslyRetainedViewRef.measure(successCallback);
expect(nativeFabricUIManager.measure).not.toBeCalled();
expect(successCallback).not.toBeCalled();
});

it('should call FabricUIManager.measureInWindow on ref.measureInWindow', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
Expand Down Expand Up @@ -442,6 +473,37 @@ describe('ReactFabric', () => {
expect(successCallback).toHaveBeenCalledWith(10, 10, 100, 100);
});

it('should no-op if calling measureInWindow on unmounted refs', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

nativeFabricUIManager.measureInWindow.mockClear();

let viewRef;
act(() => {
ReactFabric.render(
<View
ref={ref => {
viewRef = ref;
}}
/>,
11,
);
});
const dangerouslyRetainedViewRef = viewRef;
act(() => {
ReactFabric.stopSurface(11);
});

expect(nativeFabricUIManager.measureInWindow).not.toBeCalled();
const successCallback = jest.fn();
dangerouslyRetainedViewRef.measureInWindow(successCallback);
expect(nativeFabricUIManager.measureInWindow).not.toBeCalled();
expect(successCallback).not.toBeCalled();
});

it('should support ref in ref.measureLayout', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
Expand Down Expand Up @@ -480,6 +542,119 @@ describe('ReactFabric', () => {
expect(successCallback).toHaveBeenCalledWith(1, 1, 100, 100);
});

it('should no-op if calling measureLayout on unmounted "from" ref', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

nativeFabricUIManager.measureLayout.mockClear();

let viewRef;
let otherRef;
act(() => {
ReactFabric.render(
<View>
<View
foo="bar"
ref={ref => {
viewRef = ref;
}}
/>
<View
ref={ref => {
otherRef = ref;
}}
/>
</View>,
11,
);
});
const dangerouslyRetainedOtherRef = otherRef;
act(() => {
ReactFabric.render(
<View>
<View
foo="bar"
ref={ref => {
viewRef = ref;
}}
/>
{null}
</View>,
11,
);
});

expect(nativeFabricUIManager.measureLayout).not.toBeCalled();
const successCallback = jest.fn();
const failureCallback = jest.fn();
viewRef.measureLayout(
dangerouslyRetainedOtherRef,
successCallback,
failureCallback,
);
expect(nativeFabricUIManager.measureLayout).not.toBeCalled();
expect(successCallback).not.toBeCalled();
expect(failureCallback).not.toBeCalled();
});

it('should no-op if calling measureLayout on unmounted "to" ref', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

nativeFabricUIManager.measureLayout.mockClear();

let viewRef;
let otherRef;
act(() => {
ReactFabric.render(
<View>
<View
foo="bar"
ref={ref => {
viewRef = ref;
}}
/>
<View
ref={ref => {
otherRef = ref;
}}
/>
</View>,
11,
);
});
const dangerouslyRetainedViewRef = viewRef;
act(() => {
ReactFabric.render(
<View>
{null}
<View
ref={ref => {
otherRef = ref;
}}
/>
</View>,
11,
);
});

expect(nativeFabricUIManager.measureLayout).not.toBeCalled();
const successCallback = jest.fn();
const failureCallback = jest.fn();
dangerouslyRetainedViewRef.measureLayout(
otherRef,
successCallback,
failureCallback,
);
expect(nativeFabricUIManager.measureLayout).not.toBeCalled();
expect(successCallback).not.toBeCalled();
expect(failureCallback).not.toBeCalled();
});

it('returns the correct instance and calls it in the callback', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
Expand Down