Skip to content
Permalink
Browse files

Streamline route/request path handling and split params + body in req…

…uests

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 body parsing
- I normalized path interpolation to be identical betweeen 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.
  • Loading branch information...
isiahmeadows committed Jan 22, 2019
1 parent 722a4f4 commit 75bd78edb2230e17121e88c22907ce174aca05c0
@@ -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) {
@@ -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)
@@ -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

@@ -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

@@ -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")
@@ -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
}
@@ -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
}
}
@@ -0,0 +1,23 @@
"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)
}
if (queryIndex >= 0) parseQueryString(url.slice(queryIndex + 1, queryEnd), params)
if (hashIndex >= 0) parseQueryString(url.slice(hashIndex + 1), params)
return {path: path, params: params}
}
@@ -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>
@@ -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")
})
})

0 comments on commit 75bd78e

Please sign in to comment.
You can’t perform that action at this time.