Reuse old props for the update if there are no new props
Sebastian Markbage committed
Jul 6, 2016 at 20:24 UTC
7e9b95866231e99342217593a8e3bfbae05280ef
3 files changed
+38
-10
src/renderers/noop/ReactNoop.js
+4
-2
@@ -30,8 +30,8 @@ var scheduledLowPriCallback = null;
30
const TERMINAL_TAG = 99;
31
32
type Container = { rootID: number, children: Array<Instance> };
33
-type Props = { };
34
-type Instance = { tag: 99, type: string, id: number, children: Array<Instance> };
33
+type Props = { prop: any };
34
+type Instance = { tag: 99, type: string, id: number, children: Array<Instance>, prop: any };
35
36
var instanceCounter = 0;
37
@@ -67,6 +67,7 @@ var NoopRenderer = ReactFiberReconciler({
67
id: instanceCounter++,
68
type: type,
69
children: flattenChildren(children),
70
+ prop: props.prop,
71
};
72
// Hide from unit tests
73
Object.defineProperty(inst, 'tag', { value: inst.tag, enumerable: false });
@@ -80,6 +81,7 @@ var NoopRenderer = ReactFiberReconciler({
81
82
commitUpdate(instance : Instance, oldProps : Props, newProps : Props, children : HostChildren<Instance>) : void {
83
instance.children = flattenChildren(children);
84
+ instance.prop = newProps.prop;
85
},
86
87
deleteInstance(instance : Instance) : void {
src/renderers/shared/fiber/ReactFiberCompleteWork.js
+12
-2
@@ -142,12 +142,18 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
142
case HostComponent:
143
const child = workInProgress.child;
144
const children = (child && !child.sibling) ? (child.output : ?Fiber | I) : child;
145
- const newProps = workInProgress.pendingProps;
146
- workInProgress.memoizedProps = newProps;
145
+ let newProps = workInProgress.pendingProps;
146
if (current && workInProgress.stateNode != null) {
147
// If we have an alternate, that means this is an update and we need to
148
// schedule a side-effect to do the updates.
149
const oldProps = current.memoizedProps;
150
+ // If we get updated because one of our children updated, we don't
151
+ // have newProps so we'll have to reuse them.
152
+ // TODO: Split the update API as separate for the props vs. children.
153
+ // Even better would be if children weren't special cased at all tho.
154
+ if (!newProps) {
155
+ newProps = oldProps;
156
+ }
157
const instance : I = workInProgress.stateNode;
158
if (prepareUpdate(instance, oldProps, newProps, children)) {
159
// This returns true if there was something to update.
@@ -155,11 +161,15 @@ module.exports = function<T, P, I, C>(config : HostConfig<T, P, I, C>) {
161
}
162
workInProgress.output = instance;
163
} else {
164
+ if (!newProps) {
165
+ throw new Error('We must have new props for new mounts.');
166
+ }
167
const instance = createInstance(workInProgress.type, newProps, children);
168
// TODO: This seems like unnecessary duplication.
169
workInProgress.stateNode = instance;
170
workInProgress.output = instance;
171
}
172
+ workInProgress.memoizedProps = newProps;
173
return null;
174
case CoroutineComponent:
175
return moveCoroutineToHandlerPhase(current, workInProgress);
src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js
+22
-6
@@ -21,11 +21,11 @@ describe('ReactIncremental', function() {
21
});
22
23
function div(...children) {
24
- return { type: 'div', children };
24
+ return { type: 'div', children, prop: undefined };
25
}
26
27
- function span(...children) {
28
- return { type: 'span', children };
27
+ function span(prop) {
28
+ return { type: 'span', children: [], prop };
29
}
30
31
it('can update child nodes of a host instance', function() {
@@ -60,7 +60,7 @@ describe('ReactIncremental', function() {
60
it('does not update child nodes if a flush is aborted', function() {
61
62
function Bar(props) {
63
- return <span>{props.text}</span>;
63
+ return <span prop={props.text} />;
64
}
65
66
function Foo(props) {
@@ -78,17 +78,33 @@ describe('ReactIncremental', function() {
78
ReactNoop.render(<Foo text="Hello" />);
79
ReactNoop.flush();
80
expect(ReactNoop.root.children).toEqual([
81
- div(div(span(), span()), span()),
81
+ div(div(span('Hello'), span('Hello')), span('Yo')),
82
]);
83
84
ReactNoop.render(<Foo text="World" />);
85
ReactNoop.flushLowPri(35);
86
expect(ReactNoop.root.children).toEqual([
87
- div(div(span(), span()), span()),
87
+ div(div(span('Hello'), span('Hello')), span('Yo')),
88
]);
89
90
});
91
92
+ it('updates a child even though the old props is empty', function() {
93
+ function Foo(props) {
94
+ return (
95
+ <div hidden={true}>
96
+ <span prop={1} />
97
+ </div>
98
+ );
99
+ }
100
+
101
+ ReactNoop.render(<Foo />);
102
+ ReactNoop.flush();
103
+ expect(ReactNoop.root.children).toEqual([
104
+ div(span(1)),
105
+ ]);
106
+ });
107
+
108
// TODO: Test that side-effects are not cut off when a work in progress node
109
// moves to "current" without flushing due to having lower priority. Does this
110
// even happen? Maybe a child doesn't get processed because it is lower prio?