Avoid in-place mutation of the attributes of Router's children (#169)

* Include test_helpers directory in lint task

* Declare chai global in eslint config

* Create assert-clone-of chai plugin for use in tests

* Update tests to use assertCloneOf

* Add test to verify that query parameters aren't carried over from previous route

* Avoid in-place mutation of the attributes of Router's children

All credit for this change goes to @developit (who sent it over Slack)

Author: rmacklin

.eslintrc

@@ -17,6 +17,7 @@
     }
   },
   "globals": {
+    "chai": true,
     "sinon": true,
     "expect": true
   },

package.json

@@ -15,7 +15,7 @@
     "minify": "uglifyjs $npm_package_main -cm -o $npm_package_minified_main -p relative --in-source-map ${npm_package_main}.map --source-map ${npm_package_minified_main}.map",
     "size": "size=$(gzip-size $npm_package_minified_main) && echo \"gzip size: $size / $(pretty-bytes $size)\"",
     "test": "npm-run-all lint build test:karma",
-    "lint": "eslint {src,test}",
+    "lint": "eslint {src,test,test_helpers}",
     "test:karma": "karma start --single-run",
     "test:watch": "karma start",
     "prepublish": "npm-run-all build test",

src/index.js

@@ -1,4 +1,4 @@
-import { h, Component } from 'preact';
+import { cloneElement, h, Component } from 'preact';
 import { exec, pathRankSort } from './util';
 
 let customHistory = null;
@@ -207,23 +207,24 @@ class Router extends Component {
 	}
 
 	getMatchingChildren(children, url, invoke) {
-		return children.slice().sort(pathRankSort).filter( ({ attributes }) => {
-			let path = attributes.path,
-				matches = exec(url, path, attributes);
+		return children.slice().sort(pathRankSort).map( vnode => {
+			let path = vnode.attributes.path,
+				matches = exec(url, path, vnode.attributes);
 			if (matches) {
 				if (invoke!==false) {
-					attributes.url = url;
-					attributes.matches = matches;
+					let newProps = { url, matches };
 					// copy matches onto props
 					for (let i in matches) {
 						if (matches.hasOwnProperty(i)) {
-							attributes[i] = matches[i];
+							newProps[i] = matches[i];
 						}
 					}
+					return cloneElement(vnode, newProps);
 				}
-				return true;
+				return vnode;
 			}
-		});
+			return false;
+		}).filter(Boolean);
 	}
 
 	render({ children, onChange }, { url }) {

test/dist.js

@@ -1,7 +1,11 @@
 import { h } from 'preact';
+import assertCloneOf from '../test_helpers/assert-clone-of';
+
 const router = require('../');
 const { Router, Link, route } = router;
 
+chai.use(assertCloneOf);
+
 describe('dist', () => {
 	it('should export Router, Link and route', () => {
 		expect(Router).to.be.a('function');
@@ -21,15 +25,15 @@ describe('dist', () => {
 
 			expect(
 				router.render({ children }, { url:'/foo' })
-			).to.equal(children[1]);
+			).to.be.cloneOf(children[1]);
 
 			expect(
 				router.render({ children }, { url:'/' })
-			).to.equal(children[0]);
+			).to.be.cloneOf(children[0]);
 
 			expect(
 				router.render({ children }, { url:'/foo/bar' })
-			).to.equal(children[2]);
+			).to.be.cloneOf(children[2]);
 		});
 	});
 });

test/dom.js

@@ -151,5 +151,23 @@ describe('dom', () => {
 				done();
 			}, 10);
 		});
+
+		it('should not carry over the previous value of a query parameter', () => {
+			class A {
+				render({ bar }){ return <p>bar is {bar}</p>; }
+			}
+			let routerRef;
+			mount(
+				<Router ref={r => routerRef = r}>
+					<A path="/foo" />
+				</Router>
+			);
+			route('/foo');
+			expect(routerRef.base.outerHTML).to.eql('<p>bar is </p>');
+			route('/foo?bar=5');
+			expect(routerRef.base.outerHTML).to.eql('<p>bar is 5</p>');
+			route('/foo');
+			expect(routerRef.base.outerHTML).to.eql('<p>bar is </p>');
+		});
 	});
 });

test/index.js

@@ -1,5 +1,8 @@
 import { Router, Link, route } from 'src';
 import { h } from 'preact';
+import assertCloneOf from '../test_helpers/assert-clone-of';
+
+chai.use(assertCloneOf);
 
 describe('preact-router', () => {
 	it('should export Router, Link and route', () => {
@@ -19,15 +22,15 @@ describe('preact-router', () => {
 
 			expect(
 				router.render({ children }, { url:'/foo' })
-			).to.equal(children[1]);
+			).to.be.cloneOf(children[1]);
 
 			expect(
 				router.render({ children }, { url:'/' })
-			).to.equal(children[0]);
+			).to.be.cloneOf(children[0]);
 
 			expect(
 				router.render({ children }, { url:'/foo/bar' })
-			).to.equal(children[2]);
+			).to.be.cloneOf(children[2]);
 		});
 
 		it('should support nested parameterized routes', () => {
@@ -40,16 +43,15 @@ describe('preact-router', () => {
 
 			expect(
 				router.render({ children }, { url:'/foo' })
-			).to.equal(children[0]);
+			).to.be.cloneOf(children[0]);
 
 			expect(
 				router.render({ children }, { url:'/foo/bar' })
-			).to.equal(children[1]).and.have.deep.property('attributes.bar', 'bar');
+			).to.be.cloneOf(children[1], { matches: { bar:'bar' }, url:'/foo/bar' });
 
 			expect(
 				router.render({ children }, { url:'/foo/bar/baz' })
-			).equal(children[2]).and.have.deep.property('attributes')
-				.which.contains.all.keys({ bar:'bar', baz:'baz' });
+			).be.cloneOf(children[2], { matches: { bar:'bar', baz:'baz' }, url:'/foo/bar/baz' });
 		});
 
 		it('should support default routes', () => {
@@ -62,15 +64,15 @@ describe('preact-router', () => {
 
 			expect(
 				router.render({ children }, { url:'/foo' })
-			).to.equal(children[2]);
+			).to.be.cloneOf(children[2]);
 
 			expect(
 				router.render({ children }, { url:'/' })
-			).to.equal(children[1]);
+			).to.be.cloneOf(children[1]);
 
 			expect(
 				router.render({ children }, { url:'/asdf/asdf' })
-			).to.equal(children[0]);
+			).to.be.cloneOf(children[0], { matches: {}, url:'/asdf/asdf' });
 		});
 
 		it('should support initial route prop', () => {
@@ -83,7 +85,7 @@ describe('preact-router', () => {
 
 			expect(
 				router.render({ children }, router.state)
-			).to.equal(children[2]);
+			).to.be.cloneOf(children[2]);
 
 			expect(new Router({})).to.have.deep.property('state.url', location.pathname + (location.search || ''));
 		});

test_helpers/assert-clone-of.js

@@ -0,0 +1,12 @@
+import { cloneElement } from 'preact';
+
+export default function assertCloneOf({ Assertion }) {
+	if (Assertion.__assertCloneOfMounted === true) return;
+	Assertion.__assertCloneOfMounted = true;
+
+	Assertion.addMethod('cloneOf', function(routeJsx, { matches = {}, url = this._obj.attributes.path } = {}) {
+		const vnode = this._obj;
+		const clonedRoute = cloneElement(routeJsx, { url, matches, ...matches });
+		new chai.Assertion(vnode).to.be.eql(clonedRoute);
+	});
+}