Fixes jsx key creation when state changes - Fixes #147

metal · Sep 1, 2016 · 3b52cb9 · 3b52cb9
1 parent e9f2df3
commit 3b52cb9
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 21 deletions.
diff --git a/packages/metal-jsx/src/JSXRenderer.js b/packages/metal-jsx/src/JSXRenderer.js
@@ -54,10 +54,21 @@ class JSXRenderer extends IncrementalDomRenderer {
 	 * @protected
 	 */
 	handleJSXElementOpened_({args}) {
+		let count = 0;
 		if (childrenCount.length > 0) {
-			const count = ++childrenCount[childrenCount.length - 1];
-			if (!core.isDefAndNotNull(args[1])) {
+			count = ++childrenCount[childrenCount.length - 1];
+		}
+
+		if (!core.isDefAndNotNull(args[1])) {
+			if (count) {
 				args[1] = JSXRenderer.KEY_PREFIX + count;
+			} else {
+				// If this is the first node being patched, just repeat the key it
+				// used before (if it has been used before).
+				const node = IncrementalDOM.currentPointer();
+				if (node) {
+					args[1] = node.__incrementalDOMData.key;
+				}
 			}
 		}
 		childrenCount.push(0);

diff --git a/packages/metal-jsx/test/JSXRenderer.js b/packages/metal-jsx/test/JSXRenderer.js
@@ -5,13 +5,24 @@ import Component from 'metal-component';
 import JSXRenderer from '../src/JSXRenderer';
 import JSXDataManager from '../src/JSXDataManager';
 
-class TestJSXComponent extends Component {
-}
-TestJSXComponent.DATA_MANAGER = JSXDataManager;
-TestJSXComponent.RENDERER = JSXRenderer;
+
 
 describe('JSXRenderer', function() {
-	var component;
+	let component;
+	let createdComps;
+	let TestJSXComponent;
+
+	beforeEach(function() {
+		createdComps = [];
+		class TestJSXComponentCtor extends Component {
+			created() {
+				createdComps.push(this);
+			}
+		}
+		TestJSXComponentCtor.DATA_MANAGER = JSXDataManager;
+		TestJSXComponentCtor.RENDERER = JSXRenderer;
+		TestJSXComponent = TestJSXComponentCtor;
+	});
 
 	afterEach(function() {
 		if (component) {
@@ -140,12 +151,7 @@ describe('JSXRenderer', function() {
 	});
 
 	it('should reuse component rendered after a conditionally rendered component', function(done) {
-		var createdChildren = [];
 		class ChildComponent extends TestJSXComponent {
-			constructor(...args) {
-				super(...args);
-				createdChildren.push(this);
-			}
 			render() {
 				return <span>Child</span>;
 			}
@@ -167,18 +173,56 @@ describe('JSXRenderer', function() {
 			}
 		}
 
+		class ParentComponent extends TestJSXComponent {
+			render() {
+				return <div><TestComponent /></div>;
+			}
+		}
+
+		component = new ParentComponent();
+		assert.strictEqual(4, createdComps.length);
+		assert.ok(createdComps[0] instanceof ParentComponent);
+		assert.ok(createdComps[1] instanceof TestComponent);
+		assert.ok(createdComps[2] instanceof ChildComponent);
+		assert.ok(createdComps[3] instanceof ChildComponent2);
+		assert.ok(!createdComps[0].isDisposed());
+		assert.ok(!createdComps[1].isDisposed());
+		assert.ok(!createdComps[2].isDisposed());
+		assert.ok(!createdComps[3].isDisposed());
+
+		createdComps[1].props.hide = true;
+		createdComps[1].once('stateSynced', function() {
+			assert.strictEqual(4, createdComps.length);
+			assert.ok(!createdComps[0].isDisposed());
+			assert.ok(!createdComps[1].isDisposed());
+			assert.ok(createdComps[2].isDisposed());
+			assert.ok(!createdComps[3].isDisposed());
+			done();
+		});
+	});
+
+	it('should reuse elements with keys after moving around', function(done) {
+		class TestComponent extends TestJSXComponent {
+			render() {
+				return <div>
+					<span key={this.props.switch ? 'node2' : 'node1'} />
+					<span key={this.props.switch ? 'node1' : 'node2'} />
+				</div>
+			}
+		}
+		TestComponent.PROPS = {
+			switch: {
+			}
+		}
+
 		component = new TestComponent();
-		assert.strictEqual(2, createdChildren.length);
-		assert.ok(createdChildren[0] instanceof ChildComponent);
-		assert.ok(createdChildren[1] instanceof ChildComponent2);
-		assert.ok(!createdChildren[0].isDisposed());
-		assert.ok(!createdChildren[1].isDisposed());
+		const firstChild = component.element.childNodes[0];
+		const secondChild = component.element.childNodes[1];
 
-		component.props.hide = true;
+		component.props.switch = true;
 		component.once('stateSynced', function() {
-			assert.strictEqual(2, createdChildren.length);
-			assert.ok(createdChildren[0].isDisposed());
-			assert.ok(!createdChildren[1].isDisposed());
+			assert.strictEqual(secondChild, component.element.childNodes[0]);
+			assert.strictEqual(firstChild, component.element.childNodes[1]);
 			done();
 		});
 	});