Niedzielski has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/396464 )
Change subject: Chore: add query parameter typing
......................................................................
Chore: add query parameter typing
• Refactor route#RouteParams into path and query components.
• Update the tests and home page links.
• Query string marshalling and unmarshalling will come in T178617.
Bug: T178615
Bug: T178617
Change-Id: I8f07f06354ffdb459e651958cd0da49a07f23b93
---
M package.json
M src/common/pages/home.tsx
M src/common/pages/not-found.tsx
M src/common/pages/summary.tsx
M src/common/pages/wiki.tsx
M src/common/router/route.ts
M src/common/router/router.test.ts
M src/common/router/router.ts
M src/common/router/routes.test.ts
9 files changed, 162 insertions(+), 108 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/marvin refs/changes/64/396464/1
diff --git a/package.json b/package.json
index ffabbd5..259a75e 100644
--- a/package.json
+++ b/package.json
@@ -104,7 +104,7 @@
"bundlesize": [
{
"path": "dist/public/index.*.js",
- "maxSize": "3.9KB"
+ "maxSize": "4KB"
},
{
"path": "dist/public/runtime.*.js",
diff --git a/src/common/pages/home.tsx b/src/common/pages/home.tsx
index adb4091..24b77c2 100644
--- a/src/common/pages/home.tsx
+++ b/src/common/pages/home.tsx
@@ -12,6 +12,7 @@
import Page from "../components/page/page";
import { PageTitleID } from "../models/page/title";
import Link from "../components/link/link";
+import { QueryParams } from "../router/route";
const testSummaries = [
{ title: "Banana", text: "With landscape image" },
@@ -24,7 +25,7 @@
{ title: "Carrot cake", text: "Encoding redirect (301)" },
{ title: "Cheese_cake", text: "Redirect page (302)" },
{ title: "Nonexistent_title", text: "Missing (404)" },
- { title: "Pizza", query: "foo=bar", text: "Query string" }
+ { title: "Pizza", query: { foo: "bar" }, text: "Query string" }
];
const testPages = [
@@ -42,7 +43,7 @@
text: "Redirect (external) and File page"
},
{ title: "Nonexistent_title", text: "Missing (404)" },
- { title: "Pizza", query: "foo=bar", text: "Query string" }
+ { title: "Pizza", query: { foo: "bar" }, text: "Query string" }
];
export default {
@@ -82,12 +83,14 @@
text
}: {
title: PageTitleID | string;
- query?: string;
+ query?: QueryParams;
revision?: string;
text: string;
}) => (
<li>
- <Link href={wiki.toPath({ title, revision }, query)}>
+ <Link
+ href={wiki.toPath({ path: { title, revision }, query })}
+ >
{text}
</Link>
</li>
@@ -109,11 +112,13 @@
text
}: {
title: PageTitleID | string;
- query?: string;
+ query?: QueryParams;
text: string;
}) => (
<li>
- <Link href={summary.toPath({ title }, query)}>{text}</Link>
+ <Link href={summary.toPath({ path: { title }, query })}>
+ {text}
+ </Link>
</li>
)
)}
diff --git a/src/common/pages/not-found.tsx b/src/common/pages/not-found.tsx
index ce307bb..f79738e 100644
--- a/src/common/pages/not-found.tsx
+++ b/src/common/pages/not-found.tsx
@@ -3,7 +3,7 @@
import Page from "../components/page/page";
export interface Props {
- path: string;
+ pathQuery: string;
}
// Note: visually, this page may have some similarities to the generic
@@ -13,11 +13,11 @@
export default {
status: 404,
- Component({ path }: Props): JSX.Element {
+ Component({ pathQuery }: Props): JSX.Element {
return (
<App>
<Page title="Page not found" subtitle="Error 404">
- <p>Not found: {path}</p>
+ <p>Not found: {pathQuery}</p>
</Page>
</App>
);
diff --git a/src/common/pages/summary.tsx b/src/common/pages/summary.tsx
index 229a620..4b61eb9 100644
--- a/src/common/pages/summary.tsx
+++ b/src/common/pages/summary.tsx
@@ -13,18 +13,20 @@
import { RedirectError } from "../http/fetch";
import { unmarshalPageTitleID } from
"../marshallers/page-base/page-base-unmarshaller"; // eslint-disable-line
max-len
-interface PageParams extends RouteParams {
- /**
- * When used as an input, an (unencoded, not necessarily denormalized)
- * possible PageTitleID; when used as an output of Route.toParams(), an
- * (encoded, not necessarily denormalized) PageTitlePath.
- */
- title: PageTitleID | PageTitlePath | string;
+interface PageParams extends Partial<RouteParams> {
+ path: {
+ /**
+ * When used as an input, an (unencoded, not necessarily denormalized)
+ * possible PageTitleID; when used as an output of Route.toParams(), an
+ * (encoded, not necessarily denormalized) PageTitlePath.
+ */
+ title: PageTitleID | PageTitlePath | string;
+ };
}
// undefined means random input (Route.toPath()) and {} means random output
// (Route.toParams()).
-export type Params = PageParams | { title?: undefined } | undefined;
+export type Params = PageParams | { path?: undefined } | undefined;
export interface Props {
summary: PageSummaryModel;
@@ -35,12 +37,14 @@
init?: RequestInit
): Promise<HttpResponse<PageSummaryModel>> {
return requestSummary(
- params.title === undefined
+ params.path === undefined
? { random: true, init }
- : { titlePath: params.title, init }
+ : { titlePath: params.path.title, init }
).catch(error => {
if (error instanceof RedirectError) {
- const url = summary.toPath({ title: unmarshalPageTitleID(error.url) });
+ const url = summary.toPath({
+ path: { title: unmarshalPageTitleID(error.url) }
+ });
throw new RedirectError(error.status, url);
}
diff --git a/src/common/pages/wiki.tsx b/src/common/pages/wiki.tsx
index e854d12..0a98317 100644
--- a/src/common/pages/wiki.tsx
+++ b/src/common/pages/wiki.tsx
@@ -14,22 +14,21 @@
import { Thumbnail } from "../components/thumbnail/thumbnail";
import { unmarshalPageTitleID } from
"../marshallers/page-base/page-base-unmarshaller"; // eslint-disable-line
max-len
-interface PageParams extends RouteParams {
- /**
- * When used as an input, an (unencoded, not necessarily denormalized)
- * possible PageTitleID; when used as an output of Route.toParams(), an
- * (encoded, not necessarily denormalized) PageTitlePath.
- */
- title: PageTitleID | PageTitlePath | string;
- revision?: string;
+interface PageParams extends Partial<RouteParams> {
+ path: {
+ /**
+ * When used as an input, an (unencoded, not necessarily denormalized)
+ * possible PageTitleID; when used as an output of Route.toParams(), an
+ * (encoded, not necessarily denormalized) PageTitlePath.
+ */
+ title: PageTitleID | PageTitlePath | string;
+ revision?: string;
+ };
}
// undefined means random input (Route.toPath()) and {} means random output
// (Route.toParams()).
-export type Params =
- | PageParams
- | { title?: undefined; revision?: undefined }
- | undefined;
+export type Params = PageParams | { path?: undefined } | undefined;
export interface Props {
page: PageModel;
@@ -40,20 +39,25 @@
init?: RequestInit
): Promise<HttpResponse<PageModel>> {
return requestPage(
- params.title === undefined
+ params.path === undefined
? { random: true, init }
: {
- titlePath: params.title,
+ titlePath: params.path.title,
revision:
- params.revision === undefined
+ params.path.revision === undefined
? undefined
- : parseInt(params.revision, 10),
+ : parseInt(params.path.revision, 10),
init
}
).catch(error => {
if (error instanceof RedirectError) {
const title = unmarshalPageTitleID(error.url);
- const url = wiki.toPath({ title, revision: params.revision });
+ const url = wiki.toPath({
+ path: {
+ title,
+ revision: params.path ? params.path.revision : undefined
+ }
+ });
throw new RedirectError(error.status, url);
}
diff --git a/src/common/router/route.ts b/src/common/router/route.ts
index 7754450..95e52ec 100644
--- a/src/common/router/route.ts
+++ b/src/common/router/route.ts
@@ -3,15 +3,18 @@
import HttpResponse from "../http/http-response";
/**
- * A map of path-to-regexp router path names to matches. The keys must match
- * those specified in RouteConfig.path. This map is passed to Route.toPath()
- * with unencoded keys, constructed by Route.toParams() with encoded keys, and
- * passed to PageModule.getInitialProps() with encoded keys.
+ * A map of path-to-regexp router path names to matches. The path keys must
+ * match those specified in RouteConfig.path. This map is passed to
+ * Route.toPath() with unencoded keys, constructed by Route.toParams() with
+ * encoded keys, and passed to PageModule.getInitialProps() with encoded keys.
+ * Query properties are always considered optional.
*
* Note: only strings are ever returned as outputs of Route.toParams() and all
* inputs are converted to strings in Route.toPath().
*/
-export type RouteParams = { [name: string]: string | undefined };
+export type PathParams = { [name: string]: string | undefined };
+export type QueryParams = { [name: string]: string | undefined };
+export type RouteParams = { path: PathParams; query: QueryParams };
/**
* A file that exposes a Preact UI component and optionally a function to
@@ -68,17 +71,18 @@
export interface Route<Params> extends RouteConfig {
/**
- * Generate a Params object from a given URL path or void if the path does
not
- * match. An empty object is returned for matching NoPropsRoutes.
+ * Generate a Params object from a given URL path and query string or void if
+ * the path does not match. An empty object is returned for matching
+ * NoPropsRoutes.
*/
- toParams(path: string): Params | void; // eslint-disable-line
no-use-before-define
+ toParams(pathQuery: string): Params | void; // eslint-disable-line
no-use-before-define
/** Generates a URL path from Params. */
- toPath(params: Params, query?: string): string;
+ toPath(params: Params): string; // eslint-disable-line no-use-before-define
}
export interface NoParamsRoute extends Route<undefined> {
- toPath(params?: undefined, query?: string): string;
+ toPath(params?: undefined): string; // eslint-disable-line
no-use-before-define
}
/**
@@ -93,21 +97,41 @@
function toParams(
pathRegExp: RegExp,
paramNames: pathToRegExp.Key[],
- path: string
+ pathQuery: string
) {
- const matches = pathRegExp.exec(path);
+ const matches = pathRegExp.exec(pathQuery);
if (matches) {
const [, ...paramValues] = matches;
- const params: RouteParams = {};
+ const params: RouteParams = { path: {}, query: {} };
paramNames.forEach((param, index) => {
- params[param.name] = paramValues[index];
+ params.path[param.name] = paramValues[index];
});
+ // todo: query parsing.
return params;
}
return undefined;
}
-export function newRoute<Params>({ path, page }: RouteConfig): Route<Params> {
+function toPath(
+ pathFunction: pathToRegExp.PathFunction,
+ params?: Partial<RouteParams>
+) {
+ const path = pathFunction((params && params.path) || undefined);
+ let query = "";
+ if (params && params.query && Object.keys(params.query)) {
+ // todo: real query string assembly.
+ query = "?";
+ Object.keys(params.query).forEach(name => {
+ query += `${name}=${params.query && params.query[name]}&`;
+ });
+ }
+ return `${path}${query}`;
+}
+
+export function newRoute<Params extends Partial<RouteParams> | undefined>({
+ path,
+ page
+}: RouteConfig): Route<Params> {
const paramNames: pathToRegExp.Key[] = [];
const pathRegExp = pathToRegExp(
path,
@@ -115,11 +139,11 @@
// Allow query parameters.
{ endsWith: "?" }
);
- const toPath = pathToRegExp.compile(path);
+ const pathFunction = pathToRegExp.compile(path);
return {
path,
page,
- toParams: path => toParams(pathRegExp, paramNames, path),
- toPath: (path, query) => `${toPath(path)}${query ? `?${query}` : ""}`
+ toParams: pathQuery => toParams(pathRegExp, paramNames, pathQuery),
+ toPath: params => toPath(pathFunction, params)
} as Route<Params>;
}
diff --git a/src/common/router/router.test.ts b/src/common/router/router.test.ts
index 4fe9b65..2e00894 100644
--- a/src/common/router/router.test.ts
+++ b/src/common/router/router.test.ts
@@ -40,7 +40,7 @@
.route("/404")
.then(res => {
assert.deepEqual(res.status, 404);
- assert.deepEqual(res.props.path, "/404");
+ assert.deepEqual(res.props.pathQuery, "/404");
});
});
@@ -85,7 +85,7 @@
.route("/wiki/Nonexistent_title")
.then(rsp => {
assert.deepEqual(rsp.status, 404);
- assert.deepEqual(rsp.props.path, "/wiki/Nonexistent_title");
+ assert.deepEqual(rsp.props.pathQuery, "/wiki/Nonexistent_title");
});
});
diff --git a/src/common/router/router.ts b/src/common/router/router.ts
index 21326c2..b5fc5f4 100644
--- a/src/common/router/router.ts
+++ b/src/common/router/router.ts
@@ -58,8 +58,8 @@
// todo: can we load this page dynamically instead? Many users will never even
// see a 404.
-function respondNotFound(path: string): Promise<RouteResponse<any>> {
- const props: NotFoundProps = { path };
+function respondNotFound(pathQuery: string): Promise<RouteResponse<any>> {
+ const props: NotFoundProps = { pathQuery };
return Promise.resolve({
status: notFoundPage.status,
Component: notFoundPage.Component,
@@ -68,13 +68,16 @@
});
}
-function respondError(path: string, error: Error): Promise<RouteResponse<any>>
{
+function respondError(
+ pathQuery: string,
+ error: Error
+): Promise<RouteResponse<any>> {
// Throw up RedirectErrors so that they can be handled by the server/client
// appropriately
if (error instanceof RedirectError) throw error;
if (error instanceof ClientError && error.status === 404) {
- return respondNotFound(path);
+ return respondNotFound(pathQuery);
}
console.error(`${error.message}\n${error.stack}`); // eslint-disable-line
no-console
@@ -94,16 +97,16 @@
requestPageModule: RequestPageModule<any, any> = requestPageModuleChunk
) {
return {
- route(path: string): Promise<RouteResponse<any>> {
+ route(pathQuery: string): Promise<RouteResponse<any>> {
for (const route of routes) {
- const params = route.toParams(path);
+ const params = route.toParams(pathQuery);
if (params) {
return respond(requestPageModule, route, params).catch(error =>
- respondError(path, error)
+ respondError(pathQuery, error)
);
}
}
- return respondNotFound(path);
+ return respondNotFound(pathQuery);
}
};
}
diff --git a/src/common/router/routes.test.ts b/src/common/router/routes.test.ts
index 44447bc..18d9ac2 100644
--- a/src/common/router/routes.test.ts
+++ b/src/common/router/routes.test.ts
@@ -10,7 +10,7 @@
} from "./routes";
import { Route, RouteParams } from "./route";
-interface TestParams<Params extends RouteParams | undefined> {
+interface TestParams<Params extends Partial<RouteParams> | undefined> {
name: string;
route: Route<Params>;
// The raw path. Only path-to-regexp knows how to construct a path from
params
@@ -21,19 +21,22 @@
params: Params;
}
-function testPathParams<Params extends RouteParams | undefined>({
- name,
- route,
- path,
- params
-}: TestParams<Params>) {
- it(`${name} path and parameter types are in sync`, () => {
- const expected: RouteParams = {};
- Object.keys((params as RouteParams) || {}).forEach(name => {
- const value = params && params[name];
- expected[name] =
- value === undefined ? undefined : encodeURIComponent(value);
- });
+function testPathParams({ name, route, path, params }: TestParams<any>) {
+ it(`${name} path and route parameter types are in sync`, () => {
+ const expected: RouteParams = { path: {}, query: {} };
+ if (params) {
+ Object.keys(params.path || {}).forEach(name => {
+ const value = params.path[name];
+ expected.path[name] =
+ value === undefined ? undefined : encodeURIComponent(value);
+ });
+
+ Object.keys(params.query || {}).forEach(name => {
+ const value = params.query[name];
+ expected.query[name] =
+ value === undefined ? undefined : encodeURIComponent(value);
+ });
+ }
const path = route.toPath(params);
const result = route.toParams(path);
@@ -41,11 +44,18 @@
});
it(`${name} unescaped path matches`, () => {
- const expected: RouteParams = {};
- Object.keys((params as RouteParams) || {}).forEach(name => {
- const value = params && params[name];
- expected[name] = value;
- });
+ const expected: RouteParams = { path: {}, query: {} };
+ if (params) {
+ Object.keys(params.path || {}).forEach(name => {
+ const value = params.path[name];
+ expected.path[name] = value;
+ });
+
+ Object.keys(params.query || {}).forEach(name => {
+ const value = params.query[name];
+ expected.query[name] = value;
+ });
+ }
const result = route.toParams(path);
assert.deepStrictEqual(result, expected);
@@ -63,81 +73,83 @@
name: "wiki (title)",
route: wiki,
path: "/wiki/title",
- params: { title: "title", revision: undefined }
+ params: { path: { title: "title", revision: undefined } }
},
{
name: "wiki (title, revision)",
route: wiki,
path: "/wiki/title/1",
- params: { title: "title", revision: "1" }
+ params: { path: { title: "title", revision: "1" } }
},
{
name: "wiki (title with slash)",
route: wiki,
path: "/wiki/title/text",
- params: { title: "title/text", revision: undefined }
+ params: { path: { title: "title/text", revision: undefined } }
},
{
name: "wiki (title with slash, revision)",
route: wiki,
path: "/wiki/title/text/123456789",
- params: { title: "title/text", revision: "123456789" }
+ params: { path: { title: "title/text", revision: "123456789" } }
},
{
name: "wiki (title with trailing slash)",
route: wiki,
path: "/wiki/title/",
- params: { title: "title", revision: undefined }
+ params: { path: { title: "title", revision: undefined } }
},
{
name: "wiki (title with trailing slash, revision)",
route: wiki,
path: "/wiki/title/123456789/",
- params: { title: "title", revision: "123456789" }
+ params: { path: { title: "title", revision: "123456789" } }
},
{
name: "wiki (title with slash and trailing slash)",
route: wiki,
path: "/wiki/title/text/",
- params: { title: "title/text", revision: undefined }
+ params: { path: { title: "title/text", revision: undefined } }
},
{
name: "wiki (title with slash and trailing slash, revision)",
route: wiki,
path: "/wiki/title/text/123456789/",
- params: { title: "title/text", revision: "123456789" }
+ params: { path: { title: "title/text", revision: "123456789" } }
},
{
name: "wiki (title is a slash)",
route: wiki,
path: "/wiki//",
- params: { title: "/", revision: undefined }
+ params: { path: { title: "/", revision: undefined } }
},
{
name: "wiki (title is a slash, revision)",
route: wiki,
path: "/wiki///123456789/",
- params: { title: "/", revision: "123456789" }
+ params: { path: { title: "/", revision: "123456789" } }
},
{
name: "wiki (title is a question mark)",
route: wiki,
path: "/wiki/%3f",
- params: { title: "%3f", revision: undefined }
+ params: { path: { title: "%3f", revision: undefined } }
},
{
name: "wiki (title is a question mark, revision)",
route: wiki,
path: "/wiki/%3f/123456789/",
- params: { title: "%3f", revision: "123456789" }
+ params: { path: { title: "%3f", revision: "123456789" } }
},
{
name: "wiki (title with every supported character class)",
route: wiki,
path: "/wiki/ %!\"$&'()*,-./0:;=@A\\^_`a~\x80+",
params: {
- title: " %!\"$&'()*,-./0:;=@A\\^_`a~\x80+",
- revision: undefined
+ path: {
+ title: " %!\"$&'()*,-./0:;=@A\\^_`a~\x80+",
+ revision: undefined
+ }
}
},
{
@@ -145,51 +157,53 @@
route: wiki,
path: "/wiki/ %!\"$&'()*,-./0:;=@A\\^_`a~\x80+/123456789",
params: {
- title: " %!\"$&'()*,-./0:;=@A\\^_`a~\x80+",
- revision: "123456789"
+ path: {
+ title: " %!\"$&'()*,-./0:;=@A\\^_`a~\x80+",
+ revision: "123456789"
+ }
}
},
{
name: "summary (title)",
route: summary,
path: "/page/summary/title",
- params: { title: "title" }
+ params: { path: { title: "title" } }
},
{
name: "summary (title with slash)",
route: summary,
path: "/page/summary/title/text",
- params: { title: "title/text" }
+ params: { path: { title: "title/text" } }
},
{
name: "summary (title with trailing slash)",
route: summary,
path: "/page/summary/title/",
- params: { title: "title" }
+ params: { path: { title: "title" } }
},
{
name: "summary (title is a slash)",
route: summary,
path: "/page/summary//",
- params: { title: "/" }
+ params: { path: { title: "/" } }
},
{
name: "summary (title with slash and trailing slash)",
route: summary,
path: "/page/summary/title/text/",
- params: { title: "title/text" }
+ params: { path: { title: "title/text" } }
},
{
name: "summary (title is a question mark)",
route: summary,
path: "/page/summary/%3f",
- params: { title: "%3f" }
+ params: { path: { title: "%3f" } }
},
{
name: "summary (title with every supported character class)",
route: summary,
path: "/page/summary/ %!\"$&'()*,-./0:;=@A\\^_`a~\x80+",
- params: { title: " %!\"$&'()*,-./0:;=@A\\^_`a~\x80+" }
+ params: { path: { title: " %!\"$&'()*,-./0:;=@A\\^_`a~\x80+" } }
},
{
name: "random (wiki)",
--
To view, visit https://gerrit.wikimedia.org/r/396464
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8f07f06354ffdb459e651958cd0da49a07f23b93
Gerrit-PatchSet: 1
Gerrit-Project: marvin
Gerrit-Branch: master
Gerrit-Owner: Niedzielski <[email protected]>
Gerrit-Reviewer: Sniedzielski <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
