Skip to content

Commit

Permalink
[fix] ResizeObserve only on elements using 'onLayout' prop
Browse files Browse the repository at this point in the history 
Only observe nodes when the 'onLayout' prop is specified on the
element. Fixes performance regression for browsers that rely on
MutationObserver-based shim for ResizeObserver.

Fix #1128
Close #1129
  • Loading branch information
@comp615 @necolas
comp615 authored and necolas committed Oct 10, 2018
1 parent 1f3a77d commit d31bdf2
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions packages/react-native-web/src/modules/applyLayout/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,9 @@ const applyLayout = Component => {
function componentDidMount() {
this._layoutState = emptyObject;
this._isMounted = true;
observe(this);
if (this.props.onLayout) {
observe(this);
}
}
);

Expand All @@ -116,7 +118,9 @@ const applyLayout = Component => {
componentWillUnmount,
function componentWillUnmount() {
this._isMounted = false;
unobserve(this);
if (this.props.onLayout) {
unobserve(this);

This comment has been minimized.

Sign in to view

Copy link
@bloodyowl

bloodyowl Oct 10, 2018

Contributor

would't the following case cause a memory leak?

render(<Component onLayout={() => {}} />, x);
render(<Component  />, x);
unmountComponentAtNode(x);

This comment has been minimized.

Sign in to view

Copy link
@necolas

necolas Oct 10, 2018

Owner

Don't forget to unmount your root component?

This comment has been minimized.

Sign in to view

Copy link
@bloodyowl

bloodyowl Oct 10, 2018

Contributor

I wasn't clear, sorry:

render() actually preserves the instance, the repro case I was trying to show was the following:

  • Component is mounted with a onLayout prop
  • Component receives new props without onLayout
  • Component is unmounted

In this case, it seems that observe is called in componentWillMount, but unobserve never is, because at the time it's executed, this.props.onLayout is undefined.

A solution for this would be to make observe return the unobserve function and to store it in the instance, then call it if present in componentWillUnmount.

This comment has been minimized.

Sign in to view

Copy link
@necolas

necolas Oct 10, 2018

Owner

Yeah that would be more explicit. But unobserve is called when the component updates and if the onLayout prop is removed

This comment has been minimized.

Sign in to view

Copy link
@bloodyowl

bloodyowl Oct 10, 2018

Contributor

oh right sorry, I only saw the diff subset 😅
}
}
);

Expand Down

0 comments on commit d31bdf2

Please sign in to comment.