From 6780c98b4ef789f7af89322626a3c9a083c21a06 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 31 Jan 2017 18:03:52 -0800 Subject: [PATCH 1/6] Pass key as parameter to createChild, updateSlot, et al Instead of reading the key from the element/portal/coroutine, accept the key as a parameter so that the caller override the key if needed. E.g. for maps as children. --- src/renderers/shared/fiber/ReactChildFiber.js | 83 ++++++++++++------- src/renderers/shared/fiber/ReactFiber.js | 24 ++++-- 2 files changed, 70 insertions(+), 37 deletions(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 646007ceff6..ff98846b763 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -291,11 +291,12 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber : Fiber, current : ?Fiber, element : ReactElement, + key : string | null, priority : PriorityLevel ) : Fiber { if (current == null || current.type !== element.type) { // Insert - const created = createFiberFromElement(element, priority); + const created = createFiberFromElement(element, key, priority); created.ref = coerceRef(current, element); created.return = returnFiber; return created; @@ -317,12 +318,13 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber : Fiber, current : ?Fiber, coroutine : ReactCoroutine, + key : null | string, priority : PriorityLevel ) : Fiber { // TODO: Should this also compare handler to determine whether to reuse? if (current == null || current.tag !== CoroutineComponent) { // Insert - const created = createFiberFromCoroutine(coroutine, priority); + const created = createFiberFromCoroutine(coroutine, key, priority); created.return = returnFiber; return created; } else { @@ -359,6 +361,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber : Fiber, current : ?Fiber, portal : ReactPortal, + key : null | string, priority : PriorityLevel ) : Fiber { if ( @@ -368,7 +371,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { current.stateNode.implementation !== portal.implementation ) { // Insert - const created = createFiberFromPortal(portal, priority); + const created = createFiberFromPortal(portal, key, priority); created.return = returnFiber; return created; } else { @@ -403,6 +406,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { function createChild( returnFiber : Fiber, newChild : any, + forceKey : null | string, priority : PriorityLevel ) : ?Fiber { if (typeof newChild === 'string' || typeof newChild === 'number') { @@ -417,14 +421,16 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { if (typeof newChild === 'object' && newChild !== null) { switch (newChild.$$typeof) { case REACT_ELEMENT_TYPE: { - const created = createFiberFromElement(newChild, priority); + const key = forceKey !== null ? forceKey : newChild.key; + const created = createFiberFromElement(newChild, key, priority); created.ref = coerceRef(null, newChild); created.return = returnFiber; return created; } case REACT_COROUTINE_TYPE: { - const created = createFiberFromCoroutine(newChild, priority); + const key = forceKey !== null ? forceKey : newChild.key; + const created = createFiberFromCoroutine(newChild, key, priority); created.return = returnFiber; return created; } @@ -437,7 +443,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } case REACT_PORTAL_TYPE: { - const created = createFiberFromPortal(newChild, priority); + const key = forceKey !== null ? forceKey : newChild.key; + const created = createFiberFromPortal(newChild, key, priority); created.return = returnFiber; return created; } @@ -459,17 +466,18 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber : Fiber, oldFiber : ?Fiber, newChild : any, + forceKey : string | null, priority : PriorityLevel ) : ?Fiber { // Update the fiber if the keys match, otherwise return null. - const key = oldFiber ? oldFiber.key : null; + const oldKey = oldFiber ? oldFiber.key : null; if (typeof newChild === 'string' || typeof newChild === 'number') { // Text nodes doesn't have keys. If the previous node is implicitly keyed // we can continue to replace it without aborting even if it is not a text // node. - if (key !== null) { + if (oldKey !== null) { return null; } return updateTextNode(returnFiber, oldFiber, '' + newChild, priority); @@ -478,16 +486,18 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { if (typeof newChild === 'object' && newChild !== null) { switch (newChild.$$typeof) { case REACT_ELEMENT_TYPE: { - if (newChild.key === key) { - return updateElement(returnFiber, oldFiber, newChild, priority); + const key = forceKey !== null ? forceKey : newChild.key; + if (key === oldKey) { + return updateElement(returnFiber, oldFiber, newChild, key, priority); } else { return null; } } case REACT_COROUTINE_TYPE: { - if (newChild.key === key) { - return updateCoroutine(returnFiber, oldFiber, newChild, priority); + const key = forceKey !== null ? forceKey : newChild.key; + if (key === oldKey) { + return updateCoroutine(returnFiber, oldFiber, newChild, key, priority); } else { return null; } @@ -497,6 +507,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { // Yields doesn't have keys. If the previous node is implicitly keyed // we can continue to replace it without aborting even if it is not a // yield. + const key = forceKey !== null ? forceKey : newChild.key; if (key === null) { return updateYield(returnFiber, oldFiber, newChild, priority); } else { @@ -505,8 +516,9 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } case REACT_PORTAL_TYPE: { - if (newChild.key === key) { - return updatePortal(returnFiber, oldFiber, newChild, priority); + const key = forceKey !== null ? forceKey : newChild.key; + if (key === oldKey) { + return updatePortal(returnFiber, oldFiber, newChild, key, priority); } else { return null; } @@ -516,7 +528,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { if (isArray(newChild) || getIteratorFn(newChild)) { // Fragments doesn't have keys so if the previous key is implicit we can // update it. - if (key !== null) { + if (oldKey !== null) { return null; } return updateFragment(returnFiber, oldFiber, newChild, priority); @@ -533,6 +545,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber : Fiber, newIdx : number, newChild : any, + forceKey : string | null, priority : PriorityLevel ) : ?Fiber { @@ -546,17 +559,19 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { if (typeof newChild === 'object' && newChild !== null) { switch (newChild.$$typeof) { case REACT_ELEMENT_TYPE: { + const key = forceKey !== null ? forceKey : newChild.key; const matchedFiber = existingChildren.get( - newChild.key === null ? newIdx : newChild.key + key === null ? newIdx : key ) || null; - return updateElement(returnFiber, matchedFiber, newChild, priority); + return updateElement(returnFiber, matchedFiber, newChild, key, priority); } case REACT_COROUTINE_TYPE: { + const key = forceKey !== null ? forceKey : newChild.key; const matchedFiber = existingChildren.get( - newChild.key === null ? newIdx : newChild.key + key === null ? newIdx : key ) || null; - return updateCoroutine(returnFiber, matchedFiber, newChild, priority); + return updateCoroutine(returnFiber, matchedFiber, newChild, key, priority); } case REACT_YIELD_TYPE: { @@ -567,10 +582,11 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } case REACT_PORTAL_TYPE: { + const key = forceKey !== null ? forceKey : newChild.key; const matchedFiber = existingChildren.get( - newChild.key === null ? newIdx : newChild.key + key === null ? newIdx : key ) || null; - return updatePortal(returnFiber, matchedFiber, newChild, priority); + return updatePortal(returnFiber, matchedFiber, newChild, key, priority); } } @@ -587,20 +603,17 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { function warnOnDuplicateKey( child : mixed, + key : string | null, knownKeys : Set | null ) : Set | null { if (__DEV__) { - if (typeof child !== 'object' || child == null) { + if (key === null || typeof child !== 'object' || child == null) { return knownKeys; } switch (child.$$typeof) { case REACT_ELEMENT_TYPE: case REACT_COROUTINE_TYPE: case REACT_PORTAL_TYPE: - const key = child.key; - if (typeof key !== 'string') { - break; - } if (knownKeys == null) { knownKeys = new Set(); knownKeys.add(key); @@ -656,7 +669,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { let knownKeys = null; for (let i = 0; i < newChildren.length; i++) { const child = newChildren[i]; - knownKeys = warnOnDuplicateKey(child, knownKeys); + const key = (child && child.key) || null; + knownKeys = warnOnDuplicateKey(child, key, knownKeys); } } @@ -680,6 +694,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber, oldFiber, newChildren[newIdx], + null, priority ); if (!newFiber) { @@ -727,6 +742,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { const newFiber = createChild( returnFiber, newChildren[newIdx], + null, priority ); if (!newFiber) { @@ -754,6 +770,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber, newIdx, newChildren[newIdx], + null, priority ); if (newFiber) { @@ -812,7 +829,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { let step = newChildren.next(); for (; !step.done; step = newChildren.next()) { const child = step.value; - knownKeys = warnOnDuplicateKey(child, knownKeys); + const key = (child && child.key) || null; + knownKeys = warnOnDuplicateKey(child, key, knownKeys); } } @@ -843,6 +861,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber, oldFiber, step.value, + null, priority ); if (!newFiber) { @@ -890,6 +909,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { const newFiber = createChild( returnFiber, step.value, + null, priority ); if (!newFiber) { @@ -917,6 +937,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber, newIdx, step.value, + null, priority ); if (newFiber) { @@ -1008,7 +1029,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { child = child.sibling; } - const created = createFiberFromElement(element, priority); + const created = createFiberFromElement(element, key, priority); created.ref = coerceRef(currentFirstChild, element); created.return = returnFiber; return created; @@ -1042,7 +1063,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { child = child.sibling; } - const created = createFiberFromCoroutine(coroutine, priority); + const created = createFiberFromCoroutine(coroutine, key, priority); created.return = returnFiber; return created; } @@ -1105,7 +1126,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { child = child.sibling; } - const created = createFiberFromPortal(portal, priority); + const created = createFiberFromPortal(portal, key, priority); created.return = returnFiber; return created; } diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index 6af37269e75..eb6debc8687 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -297,13 +297,17 @@ exports.createHostRootFiber = function() : Fiber { return fiber; }; -exports.createFiberFromElement = function(element : ReactElement, priorityLevel : PriorityLevel) : Fiber { +exports.createFiberFromElement = function( + element : ReactElement, + key : null | string, + priorityLevel : PriorityLevel +) : Fiber { let owner = null; if (__DEV__) { owner = element._owner; } - const fiber = createFiberFromElementType(element.type, element.key, owner); + const fiber = createFiberFromElementType(element.type, key, owner); fiber.pendingProps = element.props; fiber.pendingWorkPriority = priorityLevel; @@ -388,8 +392,12 @@ function createFiberFromElementType( exports.createFiberFromElementType = createFiberFromElementType; -exports.createFiberFromCoroutine = function(coroutine : ReactCoroutine, priorityLevel : PriorityLevel) : Fiber { - const fiber = createFiber(CoroutineComponent, coroutine.key); +exports.createFiberFromCoroutine = function( + coroutine : ReactCoroutine, + key : null | string, + priorityLevel : PriorityLevel +) : Fiber { + const fiber = createFiber(CoroutineComponent, key); fiber.type = coroutine.handler; fiber.pendingProps = coroutine; fiber.pendingWorkPriority = priorityLevel; @@ -401,8 +409,12 @@ exports.createFiberFromYield = function(yieldNode : ReactYield, priorityLevel : return fiber; }; -exports.createFiberFromPortal = function(portal : ReactPortal, priorityLevel : PriorityLevel) : Fiber { - const fiber = createFiber(HostPortal, portal.key); +exports.createFiberFromPortal = function( + portal : ReactPortal, + key : null | string, + priorityLevel : PriorityLevel +) : Fiber { + const fiber = createFiber(HostPortal, key); fiber.pendingProps = portal.children || []; fiber.pendingWorkPriority = priorityLevel; fiber.stateNode = { From 38751fa01d3dd1fd963ce8956475a2fbc45b6ec0 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 1 Feb 2017 16:45:19 -0800 Subject: [PATCH 2/6] Allow text nodes, yields, and fragments to have keys Typically these aren't keyed because there's no key on the child object. But they are keyed if you use a Map. --- src/renderers/shared/fiber/ReactChildFiber.js | 85 ++++++++++--------- src/renderers/shared/fiber/ReactFiber.js | 14 ++- 2 files changed, 51 insertions(+), 48 deletions(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index ff98846b763..0099267db95 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -271,11 +271,12 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber : Fiber, current : ?Fiber, textContent : string, + key : string | null, priority : PriorityLevel ) { if (current == null || current.tag !== HostText) { // Insert - const created = createFiberFromText(textContent, priority); + const created = createFiberFromText(textContent, key, priority); created.return = returnFiber; return created; } else { @@ -340,11 +341,12 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber : Fiber, current : ?Fiber, yieldNode : ReactYield, + key : null | string, priority : PriorityLevel ) : Fiber { if (current == null || current.tag !== YieldComponent) { // Insert - const created = createFiberFromYield(yieldNode, priority); + const created = createFiberFromYield(yieldNode, key, priority); created.type = yieldNode.value; created.return = returnFiber; return created; @@ -387,11 +389,12 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { returnFiber : Fiber, current : ?Fiber, fragment : Iterable<*>, + key : null | string, priority : PriorityLevel ) : Fiber { if (current == null || current.tag !== Fragment) { // Insert - const created = createFiberFromFragment(fragment, priority); + const created = createFiberFromFragment(fragment, key, priority); created.return = returnFiber; return created; } else { @@ -410,10 +413,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { priority : PriorityLevel ) : ?Fiber { if (typeof newChild === 'string' || typeof newChild === 'number') { - // Text nodes doesn't have keys. If the previous node is implicitly keyed - // we can continue to replace it without aborting even if it is not a text - // node. - const created = createFiberFromText('' + newChild, priority); + const key = forceKey; + const created = createFiberFromText('' + newChild, key, priority); created.return = returnFiber; return created; } @@ -436,7 +437,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } case REACT_YIELD_TYPE: { - const created = createFiberFromYield(newChild, priority); + const key = forceKey; + const created = createFiberFromYield(newChild, key, priority); created.type = newChild.value; created.return = returnFiber; return created; @@ -451,7 +453,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } if (isArray(newChild) || getIteratorFn(newChild)) { - const created = createFiberFromFragment(newChild, priority); + const key = forceKey; + const created = createFiberFromFragment(newChild, key, priority); created.return = returnFiber; return created; } @@ -474,13 +477,12 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { const oldKey = oldFiber ? oldFiber.key : null; if (typeof newChild === 'string' || typeof newChild === 'number') { - // Text nodes doesn't have keys. If the previous node is implicitly keyed - // we can continue to replace it without aborting even if it is not a text - // node. - if (oldKey !== null) { + const key = forceKey; + if (key === oldKey) { + return updateTextNode(returnFiber, oldFiber, '' + newChild, key, priority); + } else { return null; } - return updateTextNode(returnFiber, oldFiber, '' + newChild, priority); } if (typeof newChild === 'object' && newChild !== null) { @@ -504,12 +506,9 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } case REACT_YIELD_TYPE: { - // Yields doesn't have keys. If the previous node is implicitly keyed - // we can continue to replace it without aborting even if it is not a - // yield. - const key = forceKey !== null ? forceKey : newChild.key; - if (key === null) { - return updateYield(returnFiber, oldFiber, newChild, priority); + const key = forceKey; + if (key === oldKey) { + return updateYield(returnFiber, oldFiber, newChild, key, priority); } else { return null; } @@ -526,12 +525,12 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } if (isArray(newChild) || getIteratorFn(newChild)) { - // Fragments doesn't have keys so if the previous key is implicit we can - // update it. - if (oldKey !== null) { + const key = forceKey; + if (key === oldKey) { + return updateFragment(returnFiber, oldFiber, newChild, key, priority); + } else { return null; } - return updateFragment(returnFiber, oldFiber, newChild, priority); } throwOnInvalidObjectType(returnFiber, newChild); @@ -550,10 +549,11 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { ) : ?Fiber { if (typeof newChild === 'string' || typeof newChild === 'number') { - // Text nodes doesn't have keys, so we neither have to check the old nor - // new node for the key. If both are text nodes, they match. - const matchedFiber = existingChildren.get(newIdx) || null; - return updateTextNode(returnFiber, matchedFiber, '' + newChild, priority); + const key = forceKey; + const matchedFiber = existingChildren.get( + key === null ? newIdx : key + ) || null; + return updateTextNode(returnFiber, matchedFiber, '' + newChild, key, priority); } if (typeof newChild === 'object' && newChild !== null) { @@ -575,10 +575,11 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } case REACT_YIELD_TYPE: { - // Yields doesn't have keys, so we neither have to check the old nor - // new node for the key. If both are yields, they match. - const matchedFiber = existingChildren.get(newIdx) || null; - return updateYield(returnFiber, matchedFiber, newChild, priority); + const key = forceKey; + const matchedFiber = existingChildren.get( + key === null ? newIdx : key + ) || null; + return updateYield(returnFiber, matchedFiber, newChild, key, priority); } case REACT_PORTAL_TYPE: { @@ -591,8 +592,11 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } if (isArray(newChild) || getIteratorFn(newChild)) { - const matchedFiber = existingChildren.get(newIdx) || null; - return updateFragment(returnFiber, matchedFiber, newChild, priority); + const key = forceKey; + const matchedFiber = existingChildren.get( + key === null ? newIdx : key + ) || null; + return updateFragment(returnFiber, matchedFiber, newChild, key, priority); } throwOnInvalidObjectType(returnFiber, newChild); @@ -977,9 +981,9 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { textContent : string, priority : PriorityLevel ) : Fiber { - // There's no need to check for keys on text nodes since we don't have a - // way to define them. - if (currentFirstChild && currentFirstChild.tag === HostText) { + // Text nodes can only have a key if they are part of a Map. + const key = null; + if (currentFirstChild && currentFirstChild.tag === HostText && currentFirstChild.key === key) { // We already have an existing node so let's just update it and delete // the rest. deleteRemainingChildren(returnFiber, currentFirstChild.sibling); @@ -991,7 +995,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { // The existing first child is not a text node so we need to create one // and delete the existing ones. deleteRemainingChildren(returnFiber, currentFirstChild); - const created = createFiberFromText(textContent, priority); + const created = createFiberFromText(textContent, key, priority); created.return = returnFiber; return created; } @@ -1074,7 +1078,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { yieldNode : ReactYield, priority : PriorityLevel ) : Fiber { - // There's no need to check for keys on yields since they're stateless. + // Yields can only have a key if they are part of a Map. + const key = null; let child = currentFirstChild; if (child) { if (child.tag === YieldComponent) { @@ -1088,7 +1093,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } } - const created = createFiberFromYield(yieldNode, priority); + const created = createFiberFromYield(yieldNode, key, priority); created.type = yieldNode.value; created.return = returnFiber; return created; diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index eb6debc8687..8ae32fba1a9 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -319,17 +319,15 @@ exports.createFiberFromElement = function( return fiber; }; -exports.createFiberFromFragment = function(elements : ReactFragment, priorityLevel : PriorityLevel) : Fiber { - // TODO: Consider supporting keyed fragments. Technically, we accidentally - // support that in the existing React. - const fiber = createFiber(Fragment, null); +exports.createFiberFromFragment = function(elements : ReactFragment, key : null | string, priorityLevel : PriorityLevel) : Fiber { + const fiber = createFiber(Fragment, key); fiber.pendingProps = elements; fiber.pendingWorkPriority = priorityLevel; return fiber; }; -exports.createFiberFromText = function(content : string, priorityLevel : PriorityLevel) : Fiber { - const fiber = createFiber(HostText, null); +exports.createFiberFromText = function(content : string, key : null | string, priorityLevel : PriorityLevel) : Fiber { + const fiber = createFiber(HostText, key); fiber.pendingProps = content; fiber.pendingWorkPriority = priorityLevel; return fiber; @@ -404,8 +402,8 @@ exports.createFiberFromCoroutine = function( return fiber; }; -exports.createFiberFromYield = function(yieldNode : ReactYield, priorityLevel : PriorityLevel) : Fiber { - const fiber = createFiber(YieldComponent, null); +exports.createFiberFromYield = function(yieldNode : ReactYield, key : null | string, priorityLevel : PriorityLevel) : Fiber { + const fiber = createFiber(YieldComponent, key); return fiber; }; From d11a20f9c6af6c8b61944d11d18afc7d070531ff Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 1 Feb 2017 16:48:32 -0800 Subject: [PATCH 3/6] Make test work in both Stack and Fiber Stack uses comment nodes to key text nodes, but Fiber doesn't. The test now works regardless. --- .../__tests__/ReactMultiChildText-test.js | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js b/src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js index 8b0a180fb19..d22777a1fdc 100644 --- a/src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js +++ b/src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js @@ -224,32 +224,41 @@ describe('ReactMultiChildText', () => { it('should reorder keyed text nodes', () => { spyOn(console, 'error'); + function getAlphaAndBeta(nodes) { + var alpha; + var beta; + for (var i = 0; i < nodes.length; i++) { + if (nodes[i].textContent === 'alpha') { + alpha = nodes[i]; + } else if (nodes[i].textContent === 'beta') { + beta = nodes[i]; + } + } + return [alpha, beta]; + } + var container = document.createElement('div'); ReactDOM.render(
{new Map([['a', 'alpha'], ['b', 'beta']])}
, container ); - var childNodes = container.firstChild.childNodes; - var alpha1 = childNodes[0]; - var alpha2 = childNodes[1]; - var alpha3 = childNodes[2]; - var beta1 = childNodes[3]; - var beta2 = childNodes[4]; - var beta3 = childNodes[5]; + var alphaAndBeta1 = getAlphaAndBeta(container.firstChild.childNodes); + + expect(container.textContent).toEqual('alphabeta'); + expect(alphaAndBeta1[0]).toBeDefined(); + expect(alphaAndBeta1[1]).toBeDefined(); ReactDOM.render(
{new Map([['b', 'beta'], ['a', 'alpha']])}
, container ); - childNodes = container.firstChild.childNodes; - expect(childNodes[0]).toBe(beta1); - expect(childNodes[1]).toBe(beta2); - expect(childNodes[2]).toBe(beta3); - expect(childNodes[3]).toBe(alpha1); - expect(childNodes[4]).toBe(alpha2); - expect(childNodes[5]).toBe(alpha3); + var alphaAndBeta2 = getAlphaAndBeta(container.firstChild.childNodes); + + expect(container.textContent).toEqual('betaalpha'); + expect(alphaAndBeta1[0]).toBe(alphaAndBeta2[0]); + expect(alphaAndBeta1[1]).toBe(alphaAndBeta2[1]); // Using Maps as children gives a single warning expectDev(console.error.calls.count()).toBe(1); From 69dc8e4773a4d45eb17fd82604340260c20de3bb Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 1 Feb 2017 16:54:02 -0800 Subject: [PATCH 4/6] Add support for maps as a keyed collection of children Maps have special semantics where the values are fragment children and the keys are React keys. --- src/renderers/shared/fiber/ReactChildFiber.js | 43 +++++++++++++------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 0099267db95..6504f21066f 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -673,7 +673,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { let knownKeys = null; for (let i = 0; i < newChildren.length; i++) { const child = newChildren[i]; - const key = (child && child.key) || null; + const key = (child && typeof child.key === 'string') ? child.key : null; knownKeys = warnOnDuplicateKey(child, key, knownKeys); } } @@ -822,6 +822,11 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { throw new Error('An object is not an iterable.'); } + const isMap = + // First condition is necessary to satisfy Flow. Is there a better way? + typeof newChildrenIterable.entries === 'function' && + iteratorFn === newChildrenIterable.entries; + if (__DEV__) { // First, validate keys. // We'll get a different iterator later for the main pass. @@ -832,9 +837,15 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { let knownKeys = null; let step = newChildren.next(); for (; !step.done; step = newChildren.next()) { - const child = step.value; - const key = (child && child.key) || null; - knownKeys = warnOnDuplicateKey(child, key, knownKeys); + if (isMap) { + const child = step.value[1]; + const key = '' + step.value[0]; + knownKeys = warnOnDuplicateKey(child, key, knownKeys); + } else { + const child = step.value; + const key = (child && typeof child.key === 'string') ? child.key : null; + knownKeys = warnOnDuplicateKey(child, key, knownKeys); + } } } @@ -861,11 +872,14 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { nextOldFiber = oldFiber.sibling; } } + + const newChild = isMap ? step.value[1] : step.value; + const forceKey = isMap ? '' + step.value[0] : null; const newFiber = updateSlot( returnFiber, oldFiber, - step.value, - null, + newChild, + forceKey, priority ); if (!newFiber) { @@ -910,10 +924,12 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { // If we don't have any more existing children we can choose a fast path // since the rest will all be insertions. for (; !step.done; newIdx++, step = newChildren.next()) { + const newChild = isMap ? step.value[1] : step.value; + const forceKey = isMap ? '' + step.value[0] : null; const newFiber = createChild( returnFiber, - step.value, - null, + newChild, + forceKey, priority ); if (!newFiber) { @@ -936,12 +952,14 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { // Keep scanning and use the map to restore deleted items as moves. for (; !step.done; newIdx++, step = newChildren.next()) { + const newChild = isMap ? step.value[1] : step.value; + const forceKey = isMap ? '' + step.value[0] : null; const newFiber = updateFromMap( existingChildren, returnFiber, newIdx, - step.value, - null, + newChild, + forceKey, priority ); if (newFiber) { @@ -951,9 +969,10 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { // current, that means that we reused the fiber. We need to delete // it from the child list so that we don't add it to the deletion // list. - existingChildren.delete( - newFiber.key === null ? newIdx : newFiber.key + const key = forceKey !== null ? forceKey : ( + newFiber.key !== null ? newFiber.key : newIdx ); + existingChildren.delete(key); } } lastPlacedIndex = placeChild(newFiber, lastPlacedIndex, newIdx); From baacc3e6f50703c555fa0ab80867f46a57a2cca2 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 1 Feb 2017 17:03:52 -0800 Subject: [PATCH 5/6] Add maps warning Maps are still experimental. Warn the first time a map is used. --- src/renderers/shared/fiber/ReactChildFiber.js | 38 +++++++++++++++---- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 6504f21066f..e9e235be422 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -42,6 +42,7 @@ if (__DEV__) { var { getCurrentFiberStackAddendum } = require('ReactDebugCurrentFiber'); var { getComponentName } = require('ReactFiberTreeReflection'); var warning = require('warning'); + var didWarnAboutMaps = false; } const { @@ -118,13 +119,10 @@ function throwOnInvalidObjectType(returnFiber : Fiber, newChild : Object) { ' If you meant to render a collection of children, use an array ' + 'instead or wrap the object using createFragment(object) from the ' + 'React add-ons.'; - const owner = ReactCurrentOwner.owner || returnFiber._debugOwner; - if (owner && typeof owner.tag === 'number') { - const name = getComponentName((owner : any)); - if (name) { - addendum += '\n\nCheck the render method of `' + name + '`.'; - } - } + const ownerName = getOwnerName(returnFiber); + addendum += ownerName ? + '\n\nCheck the render method of `' + ownerName + '`.' : + ''; } invariant( false, @@ -137,6 +135,16 @@ function throwOnInvalidObjectType(returnFiber : Fiber, newChild : Object) { } } +function getOwnerName(returnFiber : Fiber) : string | null { + if (__DEV__) { + const owner = ReactCurrentOwner.current || returnFiber._debugOwner; + if (owner && typeof owner.tag === 'number') { + return getComponentName((owner : any)); + } + } + return null; +} + // This wrapper function exists because I expect to clone the code in each path // to be able to optimize each path individually by branching early. This needs // a compiler or we can do it manually. Helpers that don't need this branching @@ -828,6 +836,22 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { iteratorFn === newChildrenIterable.entries; if (__DEV__) { + if (isMap && !didWarnAboutMaps) { + // Warn about experimental maps usage the first time a map is used. + const ownerName = getOwnerName(returnFiber); + const addendum = ownerName ? + '\n\nCheck the render method of `' + ownerName + '`.' : + ''; + warning( + didWarnAboutMaps, + 'Using Maps as children is not yet fully supported. It is an ' + + 'experimental feature that might be removed. Convert it to a ' + + 'sequence / iterable of keyed ReactElements instead.%s', + addendum + ); + didWarnAboutMaps = true; + } + // First, validate keys. // We'll get a different iterator later for the main pass. const newChildren = iteratorFn.call(newChildrenIterable); From dc5ff206eec2bc446df26857b53da1f61e1d0c9b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 1 Feb 2017 17:24:31 -0800 Subject: [PATCH 6/6] Run test script --- scripts/fiber/tests-failing.txt | 3 --- scripts/fiber/tests-passing-except-dev.txt | 3 --- scripts/fiber/tests-passing.txt | 2 ++ 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 06137eac049..bd9921f3fc4 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -50,8 +50,5 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js * should carry through each of the phases of setup -src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js -* should reorder keyed text nodes - src/renderers/shared/shared/__tests__/refs-test.js * Should increase refs with an increase in divs diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index decc5427129..dab94d2b081 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/scripts/fiber/tests-passing-except-dev.txt @@ -111,8 +111,5 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js * should warn about `setState` in getChildContext * should disallow nested render calls -src/renderers/shared/shared/__tests__/ReactMultiChild-test.js -* should warn for using maps as children with owner info - src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js * should warn for childContextTypes on a functional component diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 3b85124db56..5e63abf2bb8 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1511,6 +1511,7 @@ src/renderers/shared/shared/__tests__/ReactMultiChild-test.js * should replace children with different keys * should warn for duplicated array keys with component stack info * should warn for duplicated iterable keys with component stack info +* should warn for using maps as children with owner info * should reorder bailed-out children * prepares new children before unmounting old @@ -1548,6 +1549,7 @@ src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js * should correctly handle all possible children for render and update * should throw if rendering both HTML and children * should render between nested components and inline children +* should reorder keyed text nodes src/renderers/shared/shared/__tests__/ReactStateSetters-test.js * createStateSetter should update state