From 85bfd0f77daa7ef5127ed3c9032c5eb8e230acbf Mon Sep 17 00:00:00 2001 From: Isiah Meadows Date: Wed, 3 Jul 2019 06:22:25 -0400 Subject: [PATCH] Clarify pathname docs, follow spec with fragments (#2448) * Clarify pathname docs, follow spec with fragments - Valid URLs must not contain a `#` within its fragment. https://github.com/MithrilJS/mithril.js/issues/2445 - Our docs were a little confusing and misleading - `m.pathname` isn't aware of URLs, just path names. - Removed the relevant extension to `m.parseQueryString` required to support the hash parsing extension. Now we just shave it off and ignore it. - Fix support for arbitrary prefixes, so prefixes like `?#` are handled correctly. - Add a bunch of tests to cover various areas of confusion and unusual edge cases. * Update with PR [skip ci] --- docs/change-log.md | 3 ++ docs/parsePathname.md | 10 ++-- docs/parseQueryString.md | 5 +- docs/paths.md | 2 +- pathname/parse.js | 12 ++--- pathname/tests/test-compileTemplate.js | 12 ----- pathname/tests/test-parsePathname.js | 38 +++++++------- querystring/parse.js | 5 +- querystring/tests/test-parseQueryString.js | 12 ----- request/tests/test-request.js | 11 ++++ router/router.js | 25 +++++---- router/tests/test-defineRoutes.js | 61 ++-------------------- router/tests/test-getPath.js | 2 +- test-utils/tests/test-parseURL.js | 11 +++- test-utils/xhrMock.js | 3 +- 15 files changed, 85 insertions(+), 127 deletions(-) diff --git a/docs/change-log.md b/docs/change-log.md index 061c8ed5..4b696c28 100644 --- a/docs/change-log.md +++ b/docs/change-log.md @@ -43,6 +43,8 @@ - request: set `Content-Type: application/json; charset=utf-8` for all XHR methods by default, provided they have a body that's `!= null` ([#2361](https://github.com/MithrilJS/mithril.js/pull/2361), [#2421](https://github.com/MithrilJS/mithril.js/pull/2421)) - This can cause CORS issues when issuing `GET` with bodies, but you can address them through configuring CORS appropriately. - Previously, it was only set for all non-`GET` methods and only when `useBody: true` was passed (the default), and it was always set for them. Now it's automatically omitted when no body is present, so the hole is slightly broadened. +- route: query parameters in hash strings are no longer supported ([#2448](https://github.com/MithrilJS/mithril.js/pull/2448) [@isiahmeadows](https://github.com/isiahmeadows)) + - It's technically invalid in hashes, so I'd rather push people to keep in line with spec. #### News @@ -96,6 +98,7 @@ - request: autoredraw support fixed for `async`/`await` in Chrome ([#2428](https://github.com/MithrilJS/mithril.js/pull/2428) [@isiahmeadows](https://github.com/isiahmeadows)) - render: fix when attrs change with `onbeforeupdate` returning false, then remaining the same on next redraw ([#2447](https://github.com/MithrilJS/mithril.js/pull/2447) [@isiahmeadows](https://github.com/isiahmeadows)) - render: fix internal error when `onbeforeupdate` returns false and then true with new child tree ([#2447](https://github.com/MithrilJS/mithril.js/pull/2447) [@isiahmeadows](https://github.com/isiahmeadows)) +- route: arbitrary prefixes are properly supported now, including odd prefixes like `?#` and invalid prefixes like `#foo#bar` ([#2448](https://github.com/MithrilJS/mithril.js/pull/2448) [@isiahmeadows](https://github.com/isiahmeadows)) --- diff --git a/docs/parsePathname.md b/docs/parsePathname.md index 7e5db965..cabad2d3 100644 --- a/docs/parsePathname.md +++ b/docs/parsePathname.md @@ -32,11 +32,15 @@ Argument | Type | Required | Description ### How it works -The `m.parsePathname` method creates an object from a path with a possible query string and hash string. It is useful for parsing a URL into more sensible paths, and it's what [`m.route`](route.md) uses internally to normalize paths to later match them. It uses [`m.parseQueryString`](parseQueryString.md) to parse the query parameters into an object. +The `m.parsePathname` method creates an object from a path with a possible query string. It is useful for parsing a local path name into its parts, and it's what [`m.route`](route.md) uses internally to normalize paths to later match them. It uses [`m.parseQueryString`](parseQueryString.md) to parse the query parameters into an object. ```javascript -var data = m.parsePathname("/path/user?a=hello&b=world#random=hash&some=value") +var data = m.parsePathname("/path/user?a=hello&b=world") // data.path is "/path/user" -// data.params is {a: "hello", b: "world", random: "hash", some: "value"} +// data.params is {a: "hello", b: "world"} ``` + +### General-purpose URL parsing + +The method is called `parsePathname` because it applies to pathnames. If you want a general-purpose URL parser, you should use [the global `URL` class](https://developer.mozilla.org/en-US/docs/Web/API/URL) instead. diff --git a/docs/parseQueryString.md b/docs/parseQueryString.md index 8432a338..08e6d92a 100644 --- a/docs/parseQueryString.md +++ b/docs/parseQueryString.md @@ -19,13 +19,12 @@ var object = m.parseQueryString("a=1&b=2") ### Signature -`object = m.parseQueryString(string, object)` +`object = m.parseQueryString(string)` Argument | Type | Required | Description ------------ | ------------------------------------------ | -------- | --- `string` | `String` | Yes | A querystring -`object` | `Object` | No | An existing key-value map to merge values into, potentially from a previous `m.parseQueryString` call -**returns** | `Object` | | A key-value map, `object` if provided +**returns** | `Object` | | A key-value map [How to read signatures](signatures.md) diff --git a/docs/paths.md b/docs/paths.md index fc7c0a56..cc9a7639 100644 --- a/docs/paths.md +++ b/docs/paths.md @@ -123,7 +123,7 @@ m.route(document.body, "/edit/1", { }) ``` -Note that query parameters are implicit - you don't need to name them to accept them. You can match based on an existing value, like in `"/edit?type=image"`, but you don't need to use `"/edit?type=:type"` to accept the value. In fact, Mithril would treat that as you trying to literally match against `m.route.param("type") === ":type"`. Or in summary, use `m.route.param("key")` to extract parameters - it simplifies things. +Query parameters are implicitly consumed - you don't need to name them to accept them. You can match based on an existing value, like in `"/edit?type=image"`, but you don't need to use `"/edit?type=:type"` to accept the value. In fact, Mithril would treat that as you trying to literally match against `m.route.param("type") === ":type"`, so you probably don't want to do that. In short, use `m.route.param("key")` or route component attributes to read query parameters. ### Path normalization diff --git a/pathname/parse.js b/pathname/parse.js index b68fd6d8..4472c833 100644 --- a/pathname/parse.js +++ b/pathname/parse.js @@ -9,16 +9,16 @@ module.exports = function(url) { 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} + return { + path: path, + params: queryIndex < 0 + ? {} + : parseQueryString(url.slice(queryIndex + 1, queryEnd)), + } } diff --git a/pathname/tests/test-compileTemplate.js b/pathname/tests/test-compileTemplate.js index 5f7a5f9c..1fa37bdf 100644 --- a/pathname/tests/test-compileTemplate.js +++ b/pathname/tests/test-compileTemplate.js @@ -198,18 +198,6 @@ o.spec("compileTemplate", function() { o(compileTemplate("/route/:id?bar=foo")(data)).equals(false) o(data.params).deepEquals({foo: "bar"}) }) - o("checks hash params match", function() { - var data = parsePathname("/route/1#foo=bar") - o(compileTemplate("/route/:id#foo=bar")(data)).equals(true) - o(data.params).deepEquals({id: "1", foo: "bar"}) - }) - o("checks hash params mismatch", function() { - var data = parsePathname("/route/1#foo=bar") - o(compileTemplate("/route/:id#foo=1")(data)).equals(false) - o(data.params).deepEquals({foo: "bar"}) - o(compileTemplate("/route/:id#bar=foo")(data)).equals(false) - o(data.params).deepEquals({foo: "bar"}) - }) o("checks dot before dot", function() { var data = parsePathname("/file.test.png/edit") o(compileTemplate("/:file.:ext/edit")(data)).equals(true) diff --git a/pathname/tests/test-parsePathname.js b/pathname/tests/test-parsePathname.js index 85b3476d..565b0840 100644 --- a/pathname/tests/test-parsePathname.js +++ b/pathname/tests/test-parsePathname.js @@ -18,18 +18,18 @@ o.spec("parsePathname", function() { params: {a: "b", c: "d"} }) }) - o("parses hash at start", function() { + o("ignores hash at start", function() { var data = parsePathname("#a=b&c=d") o(data).deepEquals({ path: "/", - params: {a: "b", c: "d"} + params: {} }) }) - o("parses query + hash at start", function() { + o("parses query, ignores hash at start", function() { var data = parsePathname("?a=1&b=2#c=3&d=4") o(data).deepEquals({ path: "/", - params: {a: "1", b: "2", c: "3", d: "4"} + params: {a: "1", b: "2"} }) }) o("parses root", function() { @@ -46,18 +46,18 @@ o.spec("parsePathname", function() { params: {a: "b", c: "d"} }) }) - o("parses root + hash at start", function() { + o("parses root, ignores hash at start", function() { var data = parsePathname("/#a=b&c=d") o(data).deepEquals({ path: "/", - params: {a: "b", c: "d"} + params: {} }) }) - o("parses root + query + hash at start", function() { + o("parses root + query, ignores hash at start", function() { var data = parsePathname("/?a=1&b=2#c=3&d=4") o(data).deepEquals({ path: "/", - params: {a: "1", b: "2", c: "3", d: "4"} + params: {a: "1", b: "2"} }) }) o("parses route", function() { @@ -102,25 +102,25 @@ o.spec("parsePathname", function() { params: {c: "3", d: "4"} }) }) - o("parses route + query + hash", function() { + o("parses route + query, ignores hash", function() { var data = parsePathname("/route/foo?a=1&b=2#c=3&d=4") o(data).deepEquals({ path: "/route/foo", - params: {a: "1", b: "2", c: "3", d: "4"} + params: {a: "1", b: "2"} }) }) - o("deduplicates same-named params in query + hash", function() { - var data = parsePathname("/route/foo?a=1&b=2#a=3&c=4") - o(data).deepEquals({ - path: "/route/foo", - params: {a: "3", b: "2", c: "4"} - }) - }) - o("parses route + query + hash with lots of junk slashes", function() { + o("parses route + query, ignores hash with lots of junk slashes", function() { var data = parsePathname("//route/////foo//?a=1&b=2#c=3&d=4") o(data).deepEquals({ path: "/route/foo", - params: {a: "1", b: "2", c: "3", d: "4"} + params: {a: "1", b: "2"} + }) + }) + o("doesn't comprehend protocols", function() { + var data = parsePathname("https://example.com/foo/bar") + o(data).deepEquals({ + path: "/https:/example.com/foo/bar", + params: {} }) }) }) diff --git a/querystring/parse.js b/querystring/parse.js index 3fcb60cc..e8618938 100644 --- a/querystring/parse.js +++ b/querystring/parse.js @@ -2,12 +2,11 @@ // The extra `data` parameter is for if you want to append to an existing // parameters object. -module.exports = function(string, data) { - if (data == null) data = {} +module.exports = function(string) { if (string === "" || string == null) return {} if (string.charAt(0) === "?") string = string.slice(1) - var entries = string.split("&"), counters = {} + var entries = string.split("&"), counters = {}, data = {} for (var i = 0; i < entries.length; i++) { var entry = entries[i].split("=") var key = decodeURIComponent(entry[0]) diff --git a/querystring/tests/test-parseQueryString.js b/querystring/tests/test-parseQueryString.js index d34a06ee..ab7cb395 100644 --- a/querystring/tests/test-parseQueryString.js +++ b/querystring/tests/test-parseQueryString.js @@ -97,16 +97,4 @@ o.spec("parseQueryString", function() { var data = parseQueryString("a=1&b=2&a=3") o(data).deepEquals({a: "3", b: "2"}) }) - o("continues to append to arrays between calls", function() { - var data = {} - parseQueryString("a[]=1&a[]=2", data) - parseQueryString("a[]=3&a[]=4", data) - o(data).deepEquals({a: ["1", "2", "3", "4"]}) - }) - o("continues to append to objects between calls", function() { - var data = {} - parseQueryString("a[b]=1&a[c]=2", data) - parseQueryString("a[d]=3&a[e]=4", data) - o(data).deepEquals({a: {b: "1", c: "2", d: "3", e: "4"}}) - }) }) diff --git a/request/tests/test-request.js b/request/tests/test-request.js index 14c4e3f6..3b0990ba 100644 --- a/request/tests/test-request.js +++ b/request/tests/test-request.js @@ -73,6 +73,17 @@ o.spec("request", function() { o(data).deepEquals({a: 1}) }).then(done) }) + o("first argument keeps protocol", function(done) { + mock.$defineRoutes({ + "POST /item": function(request) { + o(request.rawUrl).equals("https://example.com/item") + return {status: 200, responseText: JSON.stringify({a: 1})} + } + }) + request("https://example.com/item", {method: "POST"}).then(function(data) { + o(data).deepEquals({a: 1}) + }).then(done) + }) o("works w/ parameterized data via GET", function(done) { mock.$defineRoutes({ "GET /item": function(request) { diff --git a/router/router.js b/router/router.js index 58af7f4e..e911c0a9 100644 --- a/router/router.js +++ b/router/router.js @@ -9,18 +9,25 @@ module.exports = function($window) { var supportsPushState = typeof $window.history.pushState === "function" var callAsync = typeof setImmediate === "function" ? setImmediate : setTimeout - function normalize(fragment) { - var data = $window.location[fragment].replace(/(?:%[a-f89][a-f0-9])+/gim, decodeURIComponent) - if (fragment === "pathname" && data[0] !== "/") data = "/" + data - return data - } - var asyncId var router = {prefix: "#!"} router.getPath = function() { - if (router.prefix.charAt(0) === "#") return normalize("hash").slice(router.prefix.length) - if (router.prefix.charAt(0) === "?") return normalize("search").slice(router.prefix.length) + normalize("hash") - return normalize("pathname").slice(router.prefix.length) + normalize("search") + normalize("hash") + // Consider the pathname holistically. The prefix might even be invalid, + // but that's not our problem. + var prefix = $window.location.hash + if (router.prefix[0] !== "#") { + prefix = $window.location.search + prefix + if (router.prefix[0] !== "?") { + prefix = $window.location.pathname + prefix + if (prefix[0] !== "/") prefix = "/" + prefix + } + } + // This seemingly useless `.concat()` speeds up the tests quite a bit, + // since the representation is consistently a relatively poorly + // optimized cons string. + return prefix.concat() + .replace(/(?:%[a-f89][a-f0-9])+/gim, decodeURIComponent) + .slice(router.prefix.length) } router.setPath = function(path, data, options) { diff --git a/router/tests/test-defineRoutes.js b/router/tests/test-defineRoutes.js index 414b6244..b85b0137 100644 --- a/router/tests/test-defineRoutes.js +++ b/router/tests/test-defineRoutes.js @@ -7,7 +7,7 @@ var Router = require("../../router/router") o.spec("Router.defineRoutes", function() { void [{protocol: "http:", hostname: "localhost"}, {protocol: "file:", hostname: "/"}].forEach(function(env) { - void ["#", "?", "", "#!", "?!", "/foo"].forEach(function(prefix) { + void ["#", "?", "", "#!", "?!", "/foo", "?#", "##"].forEach(function(prefix) { o.spec("using prefix `" + prefix + "` starting on " + env.protocol + "//" + env.hostname, function() { var $window, router, onRouteChange, onFail @@ -44,12 +44,12 @@ o.spec("Router.defineRoutes", function() { }) o("resolves to route w/ escaped unicode", function(done) { - $window.location.href = prefix + "/%C3%B6?%C3%B6=%C3%B6#%C3%B6=%C3%B6" + $window.location.href = prefix + "/%C3%B6?%C3%B6=%C3%B6" router.defineRoutes({"/ö": {data: 2}}, onRouteChange, onFail) callAsync(function() { o(onRouteChange.callCount).equals(1) - o(onRouteChange.args).deepEquals([{data: 2}, {"ö": "ö"}, "/ö?ö=ö#ö=ö", "/ö"]) + o(onRouteChange.args).deepEquals([{data: 2}, {"ö": "ö"}, "/ö?ö=ö", "/ö"]) o(onFail.callCount).equals(0) done() @@ -57,12 +57,12 @@ o.spec("Router.defineRoutes", function() { }) o("resolves to route w/ unicode", function(done) { - $window.location.href = prefix + "/ö?ö=ö#ö=ö" + $window.location.href = prefix + "/ö?ö=ö" router.defineRoutes({"/ö": {data: 2}}, onRouteChange, onFail) callAsync(function() { o(onRouteChange.callCount).equals(1) - o(onRouteChange.args).deepEquals([{data: 2}, {"ö": "ö"}, "/ö?ö=ö#ö=ö", "/ö"]) + o(onRouteChange.args).deepEquals([{data: 2}, {"ö": "ö"}, "/ö?ö=ö", "/ö"]) o(onFail.callCount).equals(0) done() @@ -138,45 +138,6 @@ o.spec("Router.defineRoutes", function() { }) }) - o("handles route with hash", function(done) { - $window.location.href = prefix + "/test#a=b&c=d" - router.defineRoutes({"/test": {data: 1}}, onRouteChange, onFail) - - callAsync(function() { - o(onRouteChange.callCount).equals(1) - o(onRouteChange.args).deepEquals([{data: 1}, {a: "b", c: "d"}, "/test#a=b&c=d", "/test"]) - o(onFail.callCount).equals(0) - - done() - }) - }) - - o("handles route with search and hash", function(done) { - $window.location.href = prefix + "/test?a=b#c=d" - router.defineRoutes({"/test": {data: 1}}, onRouteChange, onFail) - - callAsync(function() { - o(onRouteChange.callCount).equals(1) - o(onRouteChange.args).deepEquals([{data: 1}, {a: "b", c: "d"}, "/test?a=b#c=d", "/test"]) - o(onFail.callCount).equals(0) - - done() - }) - }) - - o("handles route with search and hash + duplicate params", function(done) { - $window.location.href = prefix + "/test?a=b#a=d" - router.defineRoutes({"/test": {data: 1}}, onRouteChange, onFail) - - callAsync(function() { - o(onRouteChange.callCount).equals(1) - o(onRouteChange.args).deepEquals([{data: 1}, {a: "d"}, "/test?a=b#a=d", "/test"]) - o(onFail.callCount).equals(0) - - done() - }) - }) - o("calls reject", function(done) { $window.location.href = prefix + "/test" router.defineRoutes({"/other": {data: 1}}, onRouteChange, onFail) @@ -189,18 +150,6 @@ o.spec("Router.defineRoutes", function() { }) }) - o("calls reject w/ search and hash", function(done) { - $window.location.href = prefix + "/test?a=b#c=d" - router.defineRoutes({"/other": {data: 1}}, onRouteChange, onFail) - - callAsync(function() { - o(onFail.callCount).equals(1) - o(onFail.args).deepEquals(["/test?a=b#c=d", {a: "b", c: "d"}]) - - done() - }) - }) - o("handles out of order routes", function(done) { $window.location.href = prefix + "/z/y/x" router.defineRoutes({"/z/y/x": {data: 1}, "/:a...": {data: 2}}, onRouteChange, onFail) diff --git a/router/tests/test-getPath.js b/router/tests/test-getPath.js index 9b2603fa..c405923e 100644 --- a/router/tests/test-getPath.js +++ b/router/tests/test-getPath.js @@ -6,7 +6,7 @@ var Router = require("../../router/router") o.spec("Router.getPath", function() { void [{protocol: "http:", hostname: "localhost"}, {protocol: "file:", hostname: "/"}].forEach(function(env) { - void ["#", "?", "", "#!", "?!", "/foo"].forEach(function(prefix) { + void ["#", "?", "", "#!", "?!", "/foo", "?#", "##"].forEach(function(prefix) { o.spec("using prefix `" + prefix + "` starting on " + env.protocol + "//" + env.hostname, function() { var $window, router, onRouteChange, onFail diff --git a/test-utils/tests/test-parseURL.js b/test-utils/tests/test-parseURL.js index b722cd8d..dff92683 100644 --- a/test-utils/tests/test-parseURL.js +++ b/test-utils/tests/test-parseURL.js @@ -7,7 +7,7 @@ o.spec("parseURL", function() { var root = {protocol: "http:", hostname: "localhost", port: "", pathname: "/"} o.spec("full URL", function() { - o("parses full URL", function() { + o("parses full http URL", function() { var data = parseURL("http://www.google.com:80/test?a=b#c") o(data.protocol).equals("http:") o(data.hostname).equals("www.google.com") @@ -16,6 +16,15 @@ o.spec("parseURL", function() { o(data.search).equals("?a=b") o(data.hash).equals("#c") }) + o("parses full websocket URL", function() { + var data = parseURL("ws://www.google.com:80/test?a=b#c") + o(data.protocol).equals("ws:") + o(data.hostname).equals("www.google.com") + o(data.port).equals("80") + o(data.pathname).equals("/test") + o(data.search).equals("?a=b") + o(data.hash).equals("#c") + }) o("parses full URL omitting optionals", function() { var data = parseURL("http://www.google.com") o(data.protocol).equals("http:") diff --git a/test-utils/xhrMock.js b/test-utils/xhrMock.js index cd67d26c..b4383218 100644 --- a/test-utils/xhrMock.js +++ b/test-utils/xhrMock.js @@ -36,6 +36,7 @@ module.exports = function() { } this.open = function(method, url, async, user, password) { var urlData = parseURL(url, {protocol: "http:", hostname: "localhost", port: "", pathname: "/"}) + args.rawUrl = url args.method = method args.pathname = urlData.pathname args.search = urlData.search @@ -56,7 +57,7 @@ module.exports = function() { var self = this if(!aborted) { var handler = routes[args.method + " " + args.pathname] || serverErrorHandler.bind(null, args.pathname) - var data = handler({url: args.pathname, query: args.search || {}, body: body || null}) + var data = handler({rawUrl: args.rawUrl, url: args.pathname, query: args.search || {}, body: body || null}) self.status = data.status // Match spec if (self.responseType === "json") {