React.pure automatically forwards ref (#13822)
We're not planning to encourage legacy context, and without this change, it's difficult to use pure+forwardRef together. We could special-case `pure(forwardRef(...))` but this is hopefully simpler. ```js React.pure(function(props, ref) { // ... }); ```
Sophie Alpert committed
Oct 11, 2018 at 13:04 UTC
a68ca9a5b54394bb61131f73f38329bfaeae331c
2 files changed
+161
-101
packages/react-reconciler/src/ReactFiberBeginWork.js
+4
-3
@@ -236,6 +236,7 @@ function updatePureComponent(
236
renderExpirationTime: ExpirationTime,
237
) {
238
const render = Component.render;
239
+ const ref = workInProgress.ref;
240
241
if (
242
current !== null &&
@@ -246,7 +247,7 @@ function updatePureComponent(
247
// Default to shallow comparison
248
let compare = Component.compare;
249
compare = compare !== null ? compare : shallowEqual;
249
- if (compare(prevProps, nextProps)) {
250
+ if (workInProgress.ref === current.ref && compare(prevProps, nextProps)) {
251
return bailoutOnAlreadyFinishedWork(
252
current,
253
workInProgress,
@@ -261,10 +262,10 @@ function updatePureComponent(
262
if (__DEV__) {
263
ReactCurrentOwner.current = workInProgress;
264
ReactCurrentFiber.setCurrentPhase('render');
264
- nextChildren = render(nextProps);
265
+ nextChildren = render(nextProps, ref);
266
ReactCurrentFiber.setCurrentPhase(null);
267
} else {
267
- nextChildren = render(nextProps);
268
+ nextChildren = render(nextProps, ref);
269
}
270
271
// React DevTools reads this flag.
packages/react-reconciler/src/__tests__/ReactPure-test.internal.js
+157
-98
@@ -35,14 +35,14 @@ describe('pure', () => {
35
}
36
37
// Tests should run against both the lazy and non-lazy versions of `pure`.
38
- // To make the tests work for both versions, we wrap the non-lazy verion in
38
+ // To make the tests work for both versions, we wrap the non-lazy version in
39
// a lazy function component.
40
sharedTests('normal', (...args) => {
41
const Pure = React.pure(...args);
42
- function Indirection(props) {
43
- return <Pure {...props} />;
42
+ function Indirection(props, ref) {
43
+ return <Pure {...props} ref={ref} />;
44
}
45
- return Promise.resolve(Indirection);
45
+ return Promise.resolve(React.forwardRef(Indirection));
46
});
47
sharedTests('lazy', (...args) => Promise.resolve(React.pure(...args)));
48
@@ -84,110 +84,169 @@ describe('pure', () => {
84
expect(ReactNoop.flush()).toEqual([1]);
85
expect(ReactNoop.getChildren()).toEqual([span(1)]);
86
});
87
- });
87
89
- it("does not bail out if there's a context change", async () => {
90
- const {unstable_Suspense: Suspense} = React;
91
-
92
- const CountContext = React.createContext(0);
93
-
94
- function Counter(props) {
95
- const count = CountContext.unstable_read();
96
- return <Text text={`${props.label}: ${count}`} />;
97
- }
98
- Counter = pure(Counter);
99
-
100
- class Parent extends React.Component {
101
- state = {count: 0};
102
- render() {
103
- return (
104
- <Suspense>
105
- <CountContext.Provider value={this.state.count}>
106
- <Counter label="Count" />
107
- </CountContext.Provider>
108
- </Suspense>
88
+ it("does not bail out if there's a context change", async () => {
89
+ const {unstable_Suspense: Suspense} = React;
90
+
91
+ const CountContext = React.createContext(0);
92
+
93
+ function Counter(props) {
94
+ const count = CountContext.unstable_read();
95
+ return <Text text={`${props.label}: ${count}`} />;
96
+ }
97
+ Counter = pure(Counter);
98
+
99
+ class Parent extends React.Component {
100
+ state = {count: 0};
101
+ render() {
102
+ return (
103
+ <Suspense>
104
+ <CountContext.Provider value={this.state.count}>
105
+ <Counter label="Count" />
106
+ </CountContext.Provider>
107
+ </Suspense>
108
+ );
109
+ }
110
+ }
111
+
112
+ const parent = React.createRef(null);
113
+ ReactNoop.render(<Parent ref={parent} />);
114
+ expect(ReactNoop.flush()).toEqual([]);
115
+ await Promise.resolve();
116
+ expect(ReactNoop.flush()).toEqual(['Count: 0']);
117
+ expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
118
+
119
+ // Should bail out because props have not changed
120
+ ReactNoop.render(<Parent ref={parent} />);
121
+ expect(ReactNoop.flush()).toEqual([]);
122
+ expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
123
+
124
+ // Should update because there was a context change
125
+ parent.current.setState({count: 1});
126
+ expect(ReactNoop.flush()).toEqual(['Count: 1']);
127
+ expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
128
+ });
129
+
130
+ it('accepts custom comparison function', async () => {
131
+ const {unstable_Suspense: Suspense} = React;
132
+
133
+ function Counter({count}) {
134
+ return <Text text={count} />;
135
+ }
136
+ Counter = pure(Counter, (oldProps, newProps) => {
137
+ ReactNoop.yield(
138
+ `Old count: ${oldProps.count}, New count: ${newProps.count}`,
139
);
140
+ return oldProps.count === newProps.count;
141
+ });
142
+
143
+ ReactNoop.render(
144
+ <Suspense>
145
+ <Counter count={0} />
146
+ </Suspense>,
147
+ );
148
+ expect(ReactNoop.flush()).toEqual([]);
149
+ await Promise.resolve();
150
+ expect(ReactNoop.flush()).toEqual([0]);
151
+ expect(ReactNoop.getChildren()).toEqual([span(0)]);
152
+
153
+ // Should bail out because props have not changed
154
+ ReactNoop.render(
155
+ <Suspense>
156
+ <Counter count={0} />
157
+ </Suspense>,
158
+ );
159
+ expect(ReactNoop.flush()).toEqual(['Old count: 0, New count: 0']);
160
+ expect(ReactNoop.getChildren()).toEqual([span(0)]);
161
+
162
+ // Should update because count prop changed
163
+ ReactNoop.render(
164
+ <Suspense>
165
+ <Counter count={1} />
166
+ </Suspense>,
167
+ );
168
+ expect(ReactNoop.flush()).toEqual(['Old count: 0, New count: 1', 1]);
169
+ expect(ReactNoop.getChildren()).toEqual([span(1)]);
170
+ });
171
+
172
+ it('warns for class components', () => {
173
+ class SomeClass extends React.Component {
174
+ render() {
175
+ return null;
176
+ }
177
}
111
- }
112
-
113
- const parent = React.createRef(null);
114
- ReactNoop.render(<Parent ref={parent} />);
115
- expect(ReactNoop.flush()).toEqual([]);
116
- await Promise.resolve();
117
- expect(ReactNoop.flush()).toEqual(['Count: 0']);
118
- expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
119
-
120
- // Should bail out because props have not changed
121
- ReactNoop.render(<Parent ref={parent} />);
122
- expect(ReactNoop.flush()).toEqual([]);
123
- expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
124
-
125
- // Should update because there was a context change
126
- parent.current.setState({count: 1});
127
- expect(ReactNoop.flush()).toEqual(['Count: 1']);
128
- expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
129
- });
178
+ expect(() => pure(SomeClass)).toWarnDev(
179
+ 'pure: The first argument must be a function component.',
180
+ {withoutStack: true},
181
+ );
182
+ });
183
131
- it('accepts custom comparison function', async () => {
132
- const {unstable_Suspense: Suspense} = React;
184
+ it('warns if first argument is not a function', () => {
185
+ expect(() => pure()).toWarnDev(
186
+ 'pure: The first argument must be a function component. Instead ' +
187
+ 'received: undefined',
188
+ {withoutStack: true},
189
+ );
190
+ });
191
134
- function Counter({count}) {
135
- return <Text text={count} />;
136
- }
137
- Counter = pure(Counter, (oldProps, newProps) => {
138
- ReactNoop.yield(
139
- `Old count: ${oldProps.count}, New count: ${newProps.count}`,
192
+ it('forwards ref', async () => {
193
+ const {unstable_Suspense: Suspense} = React;
194
+ const Transparent = pure((props, ref) => {
195
+ return <div ref={ref} />;
196
+ });
197
+ const divRef = React.createRef();
198
+
199
+ ReactNoop.render(
200
+ <Suspense>
201
+ <Transparent ref={divRef} />
202
+ </Suspense>,
203
);
141
- return oldProps.count === newProps.count;
204
+ ReactNoop.flush();
205
+ await Promise.resolve();
206
+ ReactNoop.flush();
207
+ expect(divRef.current.type).toBe('div');
208
});
209
144
- ReactNoop.render(
145
- <Suspense>
146
- <Counter count={0} />
147
- </Suspense>,
148
- );
149
- expect(ReactNoop.flush()).toEqual([]);
150
- await Promise.resolve();
151
- expect(ReactNoop.flush()).toEqual([0]);
152
- expect(ReactNoop.getChildren()).toEqual([span(0)]);
153
-
154
- // Should bail out because props have not changed
155
- ReactNoop.render(
156
- <Suspense>
157
- <Counter count={0} />
158
- </Suspense>,
159
- );
160
- expect(ReactNoop.flush()).toEqual(['Old count: 0, New count: 0']);
161
- expect(ReactNoop.getChildren()).toEqual([span(0)]);
162
-
163
- // Should update because count prop changed
164
- ReactNoop.render(
165
- <Suspense>
166
- <Counter count={1} />
167
- </Suspense>,
168
- );
169
- expect(ReactNoop.flush()).toEqual(['Old count: 0, New count: 1', 1]);
170
- expect(ReactNoop.getChildren()).toEqual([span(1)]);
171
- });
210
+ it('updates if only ref changes', async () => {
211
+ const {unstable_Suspense: Suspense} = React;
212
+ const Transparent = pure((props, ref) => {
213
+ return [<Text key="text" text="Text" />, <div key="div" ref={ref} />];
214
+ });
215
173
- it('warns for class components', () => {
174
- class SomeClass extends React.Component {
175
- render() {
176
- return null;
177
- }
178
- }
179
- expect(() => pure(SomeClass)).toWarnDev(
180
- 'pure: The first argument must be a function component.',
181
- {withoutStack: true},
182
- );
183
- });
216
+ const divRef = React.createRef();
217
+ const divRef2 = React.createRef();
218
+
219
+ ReactNoop.render(
220
+ <Suspense>
221
+ <Transparent ref={divRef} />
222
+ </Suspense>,
223
+ );
224
+ expect(ReactNoop.flush()).toEqual([]);
225
+ await Promise.resolve();
226
+ expect(ReactNoop.flush()).toEqual(['Text']);
227
+ expect(divRef.current.type).toBe('div');
228
+ expect(divRef2.current).toBe(null);
229
+
230
+ // Should re-render (new ref)
231
+ ReactNoop.render(
232
+ <Suspense>
233
+ <Transparent ref={divRef2} />
234
+ </Suspense>,
235
+ );
236
+ expect(ReactNoop.flush()).toEqual(['Text']);
237
+ expect(divRef.current).toBe(null);
238
+ expect(divRef2.current.type).toBe('div');
239
185
- it('warns if first argument is not a function', () => {
186
- expect(() => pure()).toWarnDev(
187
- 'pure: The first argument must be a function component. Instead ' +
188
- 'received: undefined',
189
- {withoutStack: true},
190
- );
240
+ // Should not re-render (same ref)
241
+ ReactNoop.render(
242
+ <Suspense>
243
+ <Transparent ref={divRef2} />
244
+ </Suspense>,
245
+ );
246
+ expect(ReactNoop.flush()).toEqual([]);
247
+ expect(divRef.current).toBe(null);
248
+ expect(divRef2.current.type).toBe('div');
249
+ });
250
});
251
}
252
});