-
Notifications
You must be signed in to change notification settings - Fork 46.9k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Move all markRef calls into begin phase
Certain fiber types may have a ref attached to them. The main ones are HostComponent and ClassComponent. During the render phase, we check if a ref was passed to it, and if so, we schedule a Ref effect: `markRef`. Currently, we're not consistent about whether we call `markRef` in the begin phase or the complete phase. For some fiber types, I found that `markRef` was called in both phases, causing redundant work. After some investigation, I don't believe it's necessary to call `markRef` in both the begin phase and the complete phase, as long as you don't bail out before calling `markRef`. I though that maybe it had to do with the `attemptEarlyBailoutIfNoScheduledUpdates` branch, which is a fast path that skips the regular begin phase if no new props, state, or context were passed. But if the props haven't changed (referentially — the `memo` and `shouldComponentUpdate` checks happen later), then it follows that the ref couldn't have changed either. This is true even in the old `createElement` runtime where `ref` is stored on the element instead of as a prop, because there's no way to pass a new ref to an element without also passing new props. You might argue this is a leaky assumption, but since we're shifting ref to be just a regular prop anyway, I think it's the correct way to think about it going forward. I think the pattern of calling `markRef` in the complete phase may have been left over from an earlier iteration of the implementation before the bailout logic was structured like it is today. So, I removed all the `markRef` calls from the complete phase. In the case of ScopeComponent, which had no corresponding call in the begin phase, I added one. We already had a test that asserted that a ref is reattached even if the component bails out, but I added some new ones to be extra safe. The reason I'm changing this this is because I'm working on a different change to move the ref handling logic in `coerceRef` to happen in render phase of the component that accepts the ref, instead of during the parent's reconciliation.
- Loading branch information
Showing
3 changed files
with
90 additions
and
35 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
83 changes: 83 additions & 0 deletions
83
packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| /** | ||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| * @emails react-core | ||
| */ | ||
|
|
||
| 'use strict'; | ||
|
|
||
| let React; | ||
| let Scheduler; | ||
| let ReactNoop; | ||
| let act; | ||
| let assertLog; | ||
|
|
||
| describe('ReactFiberRefs', () => { | ||
| beforeEach(() => { | ||
| jest.resetModules(); | ||
| React = require('react'); | ||
| Scheduler = require('scheduler'); | ||
| ReactNoop = require('react-noop-renderer'); | ||
| act = require('internal-test-utils').act; | ||
| assertLog = require('internal-test-utils').assertLog; | ||
| }); | ||
|
|
||
| test('ref is attached even if there are no other updates (class)', async () => { | ||
| let component; | ||
| class Component extends React.PureComponent { | ||
| render() { | ||
| Scheduler.log('Render'); | ||
| component = this; | ||
| return 'Hi'; | ||
| } | ||
| } | ||
|
|
||
| const ref1 = React.createRef(); | ||
| const ref2 = React.createRef(); | ||
| const root = ReactNoop.createRoot(); | ||
|
|
||
| // Mount with ref1 attached | ||
| await act(() => root.render(<Component ref={ref1} />)); | ||
| assertLog(['Render']); | ||
| expect(root).toMatchRenderedOutput('Hi'); | ||
| expect(ref1.current).toBe(component); | ||
| // ref2 has no value | ||
| expect(ref2.current).toBe(null); | ||
|
|
||
| // Switch to ref2, but don't update anything else. | ||
| await act(() => root.render(<Component ref={ref2} />)); | ||
| // The component did not re-render because no props changed. | ||
| assertLog([]); | ||
| expect(root).toMatchRenderedOutput('Hi'); | ||
| // But the refs still should have been swapped. | ||
| expect(ref1.current).toBe(null); | ||
| expect(ref2.current).toBe(component); | ||
| }); | ||
|
|
||
| test('ref is attached even if there are no other updates (host component)', async () => { | ||
| // This is kind of ailly test because host components never bail out if they | ||
| // receive a new element, and there's no way to update a ref without also | ||
| // updating the props, but adding it here anyway for symmetry with the | ||
| // class case above. | ||
| const ref1 = React.createRef(); | ||
| const ref2 = React.createRef(); | ||
| const root = ReactNoop.createRoot(); | ||
|
|
||
| // Mount with ref1 attached | ||
| await act(() => root.render(<div ref={ref1}>Hi</div>)); | ||
| expect(root).toMatchRenderedOutput(<div>Hi</div>); | ||
| expect(ref1.current).not.toBe(null); | ||
| // ref2 has no value | ||
| expect(ref2.current).toBe(null); | ||
|
|
||
| // Switch to ref2, but don't update anything else. | ||
| await act(() => root.render(<div ref={ref2}>Hi</div>)); | ||
| expect(root).toMatchRenderedOutput(<div>Hi</div>); | ||
| // But the refs still should have been swapped. | ||
| expect(ref1.current).toBe(null); | ||
| expect(ref2.current).not.toBe(null); | ||
| }); | ||
| }); |