Streamline route/request path handling, split params + body in requests

TODO:

- Add docs page explaining path syntax for routing and requests.
- Update commit message to really encompass all the changes.
- Update changelog to include documentation of all these changes.
- Remove this commit header.

-----

Fixes #2360
Fixes #1138
Fixes #1788 a little less hackishly
Probably fixes a few other issues I'm not aware of.

This more or less goes with @lhorie's comment here, just with a minor name
change from `query` to `params`:

#1138 (comment)

Specifically, here's what this patch entails:

- I changed `data` and `useBody` to `params` and `body` in `m.request`.
  Migration is trivial: just use `params` or `body` depending on which you
  intend to send. Most servers do actually care where the data goes, so you can
  generally pretty easily translate this accordingly. If you *really* need the
  old behavior, pass the old value in `params` and if `method === "GET"` or
  `method === "TRACE"`, also in `body`.
- I opened up all methods to have request bodies.
- I fixed `m.parseQueryString` to prefer later values over prior values and to
  ensure that objects and arrays are persisted across both hash and query param
  parsing.
- I normalized path interpolation to be identical between routes and requests.
- I no longer include interpolated values in query strings. If you need to
  duplicate values again, rename the interpolation to be a distinct property and
  pass the value you want to duplicate as it.
- I converted `m.route` to use pre-compiled routes instead of its existing
  system of dynamic runtime checking. This shouldn't have a major effect on
  performance short-term, but it'll ease the migration to built-in userland
  components and make it a little easier to reconcile. It'll also come handy for
  large numbers of routes.
- I'm throwing a few new informative errors.

I also made a few drive-by edits:

- I fixed a bug in the `Stream.HALT` warning where it warned all but the first
  usage when the intent was to warn only on first use.
- Some of the tests were erroneously using `Stream.HALT` when they should've
  been using `Stream.SKIP`. I've fixed the tests to only test that
  `Stream.HALT === Stream.SKIP` and that it only warns on first use.

-----

There is some justification to this:

- In general, it matters surprisingly more than you would expect how things
  translate to HTTP requests. So the comment there suggesting a thing that
  papers over the difference has led to plenty of confusion in both Gitter and
  in GitHub issues.

- A lot of servers expect a GET with a body and no parameters, and leaving
  `m.request` open to working with that makes it much more flexible.

- Sometimes, servers expect a POST with query parameters *instead* of a JSON
  object. I've seen this quite a bit, even with more popular REST APIs like
  Stack Overflow's.

- I've encountered a few servers that expect both parameters and a body, each
  with distinct semantic meaning, so the separation makes it much easier to
  translate into a request.

If you really don't care how it translates, just pass the same object for the
`params` and `body`. Either way, the explicit nature helps a lot.

api/router.js

@@ -36,11 +36,11 @@ module.exports = function($window, redrawService) {
 				if (payload.onmatch) {
 					Promise.resolve(payload.onmatch(params, path)).then(function(resolved) {
 						update(payload, resolved)
-					}, bail)
+					}, function () { bail(path) })
 				}
 				else update(payload, "div")
 			}
-		}, bail)
+		}, bail, defaultRoute)
 	}
 	route.set = function(path, data, options) {
 		if (lastUpdate != null) {

api/tests/test-router.js

@@ -985,7 +985,8 @@ o.spec("route", function() {
 					$window.location.href = prefix + "/a"
 					route(root, "/", {
 						"/a": {view: view},
-						"/b": {onmatch: onmatch}
+						"/b": {onmatch: onmatch},
+						"/": {view: function() {}}
 					})
 
 					o(view.callCount).equals(1)

docs/change-log.md

@@ -34,6 +34,9 @@
 - cast className using toString ([#2309](https://github.com/MithrilJS/mithril.js/pull/2309))
 - render: call attrs' hooks first, with express exception of `onbeforeupdate` to allow attrs to block components from even diffing ([#2297](https://github.com/MithrilJS/mithril.js/pull/2297))
 - API: `m.withAttr` removed. ([#2317](https://github.com/MithrilJS/mithril.js/pull/2317))
+- request: `data` has now been split to `params` and `body` and `useBody` has been removed in favor of just using `body`.
+- route, request: Interpolated arguments are URL-escaped automatically. If you want to use a raw route parameter, use a variadic parameter like in `/asset/:path.../view`. This was previously only available in `m.route` route definitions, but it's now usable in both that and where paths are accepted.
+- route, request: Interpolated arguments are *not* appended to the query string. This means `m.request({url: "/api/user/:id/get", params: {id: user.id}})` would result in a request like `GET /api/user/1/get`, not one like `GET /api/user/1/get?id=1`. If you really need it in both places, pass the same value via two separate parameters with the non-query-string parameter renamed, like in `m.request({url: "/api/user/:urlID/get", params: {id: user.id, urlID: user.id}})`.
 
 #### News
 

docs/route.md

@@ -208,7 +208,7 @@ Routing is a system that allows creating Single-Page-Applications (SPA), i.e. ap
 
 It enables seamless navigability while preserving the ability to bookmark each page individually, and the ability to navigate the application via the browser's history mechanism.
 
-Routing without page refreshes is made partially possible by the [`history.pushState`](https://developer.mozilla.org/en-US/docs/Web/API/History_API#The_pushState()_method) API. Using this API, it's possible to programmatically change the URL displayed by the browser after a page has loaded, but it's the application developer's responsibility to ensure that navigating to any given URL from a cold state (e.g. a new tab) will render the appropriate markup.
+Routing without page refreshes is made partially possible by the [`history.pushState`](https://developer.mozilla.org/en-US/docs/Web/API/History_API#The_pushState%28%29_method) API. Using this API, it's possible to programmatically change the URL displayed by the browser after a page has loaded, but it's the application developer's responsibility to ensure that navigating to any given URL from a cold state (e.g. a new tab) will render the appropriate markup.
 
 #### Routing strategies
 

index.js

@@ -19,6 +19,8 @@ m.request = requestService.request
 m.jsonp = requestService.jsonp
 m.parseQueryString = require("./querystring/parse")
 m.buildQueryString = require("./querystring/build")
+m.parsePathname = require("./pathname/parse")
+m.buildPathname = require("./pathname/build")
 m.version = "bleeding-edge"
 m.vnode = require("./render/vnode")
 m.PromisePolyfill = require("./promise/polyfill")

pathname/build.js

@@ -0,0 +1,44 @@
+"use strict"
+
+var buildQueryString = require("../querystring/build")
+
+// Returns `path` from `template` + `params`
+module.exports = function(template, params) {
+	if ((/:([^\/\.-]+)(\.{3})?:/).test(template)) {
+		throw new SyntaxError("Template parameter names *must* be separated")
+	}
+	if (params == null) return template
+	var queryIndex = template.indexOf("?")
+	var hashIndex = template.indexOf("#")
+	var queryEnd = hashIndex < 0 ? template.length : hashIndex
+	var pathEnd = queryIndex < 0 ? queryEnd : queryIndex
+	var path = template.slice(0, pathEnd)
+	var query = {}
+
+	for (var key in params) {
+		if (Object.prototype.hasOwnProperty.call(params, key)) query[key] = params[key]
+	}
+
+	var resolved = path.replace(/:([^\/\.-]+)(\.{3})?/g, function(m, key, variadic) {
+		delete query[key]
+		// If no such parameter exists, don't interpolate it.
+		if (params[key] == null) return m
+		// Escape normal parameters, but not variadic ones.
+		return variadic ? params[key] : encodeURIComponent(String(params[key]))
+	})
+
+	// In case the template substitution adds new query/hash parameters.
+	var newQueryIndex = resolved.indexOf("?")
+	var newHashIndex = resolved.indexOf("#")
+	var newQueryEnd = newHashIndex < 0 ? resolved.length : newHashIndex
+	var newPathEnd = newQueryIndex < 0 ? newQueryEnd : newQueryIndex
+	var result = resolved.slice(0, newPathEnd)
+
+	if (queryIndex >= 0) result += "?" + template.slice(queryIndex, queryEnd)
+	if (newQueryIndex >= 0) result += (queryIndex < 0 ? "?" : "&") + resolved.slice(newQueryIndex, newQueryEnd)
+	var querystring = buildQueryString(query)
+	if (querystring) result += (queryIndex < 0 && newQueryIndex < 0 ? "?" : "&") + querystring
+	if (hashIndex >= 0) result += template.slice(hashIndex)
+	if (newHashIndex >= 0) result += (hashIndex < 0 ? "" : "&") + resolved.slice(newHashIndex)
+	return result
+}

pathname/compileTemplate.js

@@ -0,0 +1,39 @@
+"use strict"
+
+// Compiles a template into a function that takes a resolved path (without query
+// strings) and returns an object containing the template parameters with their
+// parsed values. This assumes the path passed into the compiled function is the
+// `data.path` from `parsePathname`.
+module.exports = function(template) {
+	if ((/[?#]/).test(template)) {
+		throw new SyntaxError("Declared routes must not include query or hash strings")
+	}
+	if ((/:([^\/\.-]+)(\.{3})?:/).test(template)) {
+		throw new SyntaxError("Route parameter names must be separated with either `/`, `.`, or `-`")
+	}
+	var keys
+	var regexp = new RegExp("^" + template.replace(
+		// I escape literal text so people can use things like `:file.:ext` or
+		// `:lang-:locale` in routes. This is all merged into one pass so I
+		// don't also accidentally escape `-` and make it harder to detect it to
+		// ban it from template parameters.
+		/:([^\/.-]+)(\.{3})?|[\\^$*+.()|\[\]{}]/g,
+		function(m, key, variadic) {
+			if (key == null) return "\\" + m
+			if (!keys) keys = []
+			keys.push({k: key, r: Boolean(variadic)})
+			return variadic ? "(.*?)" : "([^/-]+)"
+		}
+	) + "$")
+	return function(path) {
+		// If no interpolations exist, let's skip all the ceremony
+		if (keys == null) return regexp.test(path) ? {} : undefined
+		var values = regexp.exec(path)
+		if (values == null) return
+		var params = {}
+		for (var i = 0; i < keys.length; i++) {
+			params[keys[i].k] = keys[i].r ? values[i + 1] : decodeURIComponent(values[i + 1])
+		}
+		return params
+	}
+}

pathname/parse.js

@@ -0,0 +1,25 @@
+"use strict"
+
+var parseQueryString = require("../querystring/parse")
+
+// Returns `{path, params}` from `url` + `params`
+module.exports = function(url) {
+	if (params == null) params = {}
+	var queryIndex = url.indexOf("?")
+	var hashIndex = url.indexOf("#")
+	var queryEnd = hashIndex < 0 ? url.length : hashIndex
+	var pathEnd = queryIndex < 0 ? queryEnd : queryIndex
+	var path = url.slice(0, pathEnd).replace(/\/{2,}/g, "/")
+	var params = {}
+
+	if (!path) path = "/"
+	else {
+		if (path[0] !== "/") path = "/" + path
+		if (path.length > 1 && path[path.length - 1] === "/") path = path.slice(0, -1)
+	}
+	// Note: these are reversed because `parseQueryString` appends parameters
+	// only if they don't exist. Please don't flip them.
+	if (queryIndex >= 0) parseQueryString(url.slice(queryIndex + 1, queryEnd), params)
+	if (hashIndex >= 0) parseQueryString(url.slice(hashIndex + 1), params)
+	return {path: path, params: params}
+}

pathname/tests/index.html

@@ -0,0 +1,19 @@
+<!doctype html>
+<html>
+	<head>
+		<meta charset="utf-8">
+	</head>
+	<body>
+		<script src="../../module/module.js"></script>
+		<script src="../../ospec/ospec.js"></script>
+
+		<script src="../../pathname/build.js"></script>
+		<script src="../../pathname/parse.js"></script>
+		<script src="../../pathname/parseTemplate.js"></script>
+		<script src="test-buildPathname.js"></script>
+		<script src="test-parsePathname.js"></script>
+		<script src="test-parseTemplate.js"></script>
+
+		<script>require("../../ospec/ospec").run()</script>
+	</body>
+</html>

pathname/tests/test-buildPathname.js

@@ -0,0 +1,87 @@
+"use strict"
+
+var o = require("../../ospec/ospec")
+var buildPathname = require("../../pathname/build")
+
+o.spec("buildPathname", function() {
+	o("returns path if no params", function () {
+		var string = buildPathname("/route/foo", undefined)
+
+		o(string).equals("/route/foo")
+	})
+	o("skips interpolation if no params", function () {
+		var string = buildPathname("/route/:id", undefined)
+
+		o(string).equals("/route/:id")
+	})
+	o("appends query strings", function () {
+		var string = buildPathname("/route/foo", {a: "b", c: 1})
+
+		o(string).equals("/route/foo?a=b&c=1")
+	})
+	o("inserts template parameters at end", function () {
+		var string = buildPathname("/route/:id", {id: "1"})
+
+		o(string).equals("/route/1")
+	})
+	o("inserts template parameters at beginning", function () {
+		var string = buildPathname("/:id/foo", {id: "1"})
+
+		o(string).equals("/1/foo")
+	})
+	o("inserts template parameters at middle", function () {
+		var string = buildPathname("/route/:id/foo", {id: "1"})
+
+		o(string).equals("/route/1/foo")
+	})
+	o("inserts variadic paths", function () {
+		var string = buildPathname("/route/:foo...", {foo: "id/1"})
+
+		o(string).equals("/route/id/1")
+	})
+	o("inserts variadic paths with initial slashes", function () {
+		var string = buildPathname("/route/:foo...", {foo: "/id/1"})
+
+		o(string).equals("/route//id/1")
+	})
+	o("skips template parameters at end if param missing", function () {
+		var string = buildPathname("/route/:id", {param: 1})
+
+		o(string).equals("/route/:id?param=1")
+	})
+	o("skips template parameters at beginning if param missing", function () {
+		var string = buildPathname("/:id/foo", {param: 1})
+
+		o(string).equals("/:id/foo?param=1")
+	})
+	o("skips template parameters at middle if param missing", function () {
+		var string = buildPathname("/route/:id/foo", {param: 1})
+
+		o(string).equals("/route/:id/foo?param=1")
+	})
+	o("skips variadic template parameters if param missing", function () {
+		var string = buildPathname("/route/:foo...", {param: "/id/1"})
+
+		o(string).equals("/route/:foo...?param=%2Fid%2F1")
+	})
+	o("handles escaped values", function() {
+		var data = buildPathname("/route/:foo", {"foo": ";:@&=+$,/?%#"})
+
+		o(data).equals("/route/%3B%3A%40%26%3D%2B%24%2C%2F%3F%25%23")
+	})
+	o("handles unicode", function() {
+		var data = buildPathname("/route/:ö", {"ö": "ö"})
+
+		o(data).equals("/route/%C3%B6")
+	})
+	o("handles zero", function() {
+		var string = buildPathname("/route/:a", {a: 0})
+
+		o(string).equals("/route/0")
+	})
+	o("handles false", function() {
+		var string = buildPathname("/route/:a", {a: false})
+
+		o(string).equals("/route/false")
+	})
+})