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

forwardRef() components should not re-render on deep setState() #12690

Merged
merged 3 commits into from
Apr 26, 2018
Merged

forwardRef() components should not re-render on deep setState() #12690

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
876298f
Add a failing test for forwardRef memoization
gaearon Apr 25, 2018
e3fdd87
Memoize forwardRef props and bail out on strict equality
gaearon Apr 25, 2018
60ddc00
Bail out only when ref matches the current ref
gaearon Apr 26, 2018
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
14 changes: 9 additions & 5 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,16 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(

function updateForwardRef(current, workInProgress) {
const render = workInProgress.type.render;
const nextChildren = render(
workInProgress.pendingProps,
workInProgress.ref,
);
const nextProps = workInProgress.pendingProps;
if (hasLegacyContextChanged()) {
// Normally we can bail out on props equality but if context has changed
// we don't do the bailout and we have to reuse existing props instead.
} else if (workInProgress.memoizedProps === nextProps) {
Copy link
Contributor

@bvaughn bvaughn Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should also compare workInProgress.ref

Copy link
Collaborator Author

@gaearon gaearon Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

With what?

(Sorry if I'm being dense)

Also, is it possible for a deep update to influence the value of a ref above it?

Copy link
Contributor

@bvaughn bvaughn Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also, is it possible for a deep update to influence the value of a ref above it?

I'm not sure, but it seems conceivable that a ref could be the only changed "prop" (if it's an inline arrow function for a non-PureComponent) and we might bail out when we shouldn't here?

Although in that case it probably wouldn't matter anyway.

Copy link
Contributor

@bvaughn bvaughn Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hm... maybe Foo gets new properties that causes it to recreate the ref, but only a filtered set of those props are passed through to the ForwardRef component, so from it's POV nothing has changed?

Maybe this is too contrived.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe Foo gets new properties that causes it to recreate the ref, but only a filtered set of those props are passed through to the ForwardRef component, so from it's POV nothing has changed?

OK, that sounds potentially plausible. If you find any free time to hack on this I'd appreciate a test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think maybe this is too contrived to be a real concern.

Ref callbacks would be re-invoked if the underlying instance/ref changed. If the ref callback itself changed, most likely this just means it's an arrow functions and has an identical implementation.

createRef would be more problematic, but it is only expected to be created once in the first place, so in practice shouldn't be a problem.

Maybe we should just add an inline comment saying this is technically possible but probably not a concern in practice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the ref callback itself changed, most likely this just means it's an arrow functions and has an identical implementation.

I don’t think we can rely on something like this. Otherwise we might as well never update refs.

We handled this correctly so far so I don’t think we should regress in this particular case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to write a contrived test to catch the case we're talking about, and it looks like I was mistaken to begin with. Even if props are shallowly equal, the memoizedProps and pendingProps objects themselves won't be equal- so we won't bail out- which side steps the issue I mentioned.

Sorry for the dead end. 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We chatted a bit with @acdlite and he proposed to check against current.ref. Seems like it wouldn’t hurt but I’ll think about it more tomorrow.

return bailoutOnAlreadyFinishedWork(current, workInProgress);
}
const nextChildren = render(nextProps, workInProgress.ref);
reconcileChildren(current, workInProgress, nextChildren);
memoizeProps(workInProgress, nextChildren);
memoizeProps(workInProgress, nextProps);
return workInProgress.child;
}

Expand Down
33 changes: 33 additions & 0 deletions packages/react/src/__tests__/forwardRef-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,39 @@ describe('forwardRef', () => {
expect(ref.current).toBe(null);
});

it('should not re-run the render callback on a deep setState', () => {
let inst;

class Inner extends React.Component {
render() {
ReactNoop.yield('Inner');
inst = this;
return <div ref={this.props.forwardedRef} />;
}
}

function Middle(props) {
ReactNoop.yield('Middle');
return <Inner {...props} />;
}

const Forward = React.forwardRef((props, ref) => {
ReactNoop.yield('Forward');
return <Middle {...props} forwardedRef={ref} />;
});

function App() {
ReactNoop.yield('App');
return <Forward />;
}

ReactNoop.render(<App />);
expect(ReactNoop.flush()).toEqual(['App', 'Forward', 'Middle', 'Inner']);

inst.setState({});
expect(ReactNoop.flush()).toEqual(['Inner']);
});

it('should warn if not provided a callback during creation', () => {
expect(() => React.forwardRef(undefined)).toWarnDev(
'forwardRef requires a render function but was given undefined.',
Expand Down