jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/397859 )
Change subject: Update: add query parameter parser
......................................................................
Update: add query parameter parser
Bug: T178617
Change-Id: I91c5111d5b0cdda758d1f19971fe7001a052148a
---
M package-lock.json
M package.json
M src/client/index.tsx
M src/common/pages/home.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
M src/server/index.tsx
9 files changed, 154 insertions(+), 94 deletions(-)
Approvals:
Jhernandez: Looks good to me, approved
jenkins-bot: Verified
diff --git a/package-lock.json b/package-lock.json
index be6cd49..ab97e9b 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -119,6 +119,12 @@
"@types/node": "8.0.47"
}
},
+ "@types/query-string": {
+ "version": "5.0.1",
+ "resolved":
"https://registry.npmjs.org/@types/query-string/-/query-string-5.0.1.tgz";,
+ "integrity":
"sha512-qlH3YtJ5ipp6z5SU9G0LJsBnNEZqTKPeGjNIyMcGD5Dhbooz0CCsW/sILsgb+LBRrDeTehRtQviXV2kfLm3dqg==",
+ "dev": true
+ },
"@types/serve-static": {
"version": "1.7.32",
"resolved":
"https://registry.npmjs.org/@types/serve-static/-/serve-static-1.7.32.tgz";,
@@ -1586,6 +1592,11 @@
"resolved":
"https://registry.npmjs.org/decamelize/-/decamelize-1.2.0.tgz";,
"integrity": "sha1-9lNNFRSCabIDUue+4m9QH5oZEpA=",
"dev": true
+ },
+ "decode-uri-component": {
+ "version": "0.2.0",
+ "resolved":
"https://registry.npmjs.org/decode-uri-component/-/decode-uri-component-0.2.0.tgz";,
+ "integrity": "sha1-6zkTMzRYd1y4TNGh+uBiEGu4dUU="
},
"deep-equal": {
"version": "1.0.1",
@@ -4793,6 +4804,18 @@
"prepend-http": "1.0.4",
"query-string": "4.3.4",
"sort-keys": "1.1.2"
+ },
+ "dependencies": {
+ "query-string": {
+ "version": "4.3.4",
+ "resolved":
"https://registry.npmjs.org/query-string/-/query-string-4.3.4.tgz";,
+ "integrity": "sha1-u7aTucqRXCMlFbIosaArYJBD2+s=",
+ "dev": true,
+ "requires": {
+ "object-assign": "4.1.1",
+ "strict-uri-encode": "1.1.0"
+ }
+ }
}
},
"npm-run-all": {
@@ -4873,8 +4896,7 @@
"object-assign": {
"version": "4.1.1",
"resolved":
"https://registry.npmjs.org/object-assign/-/object-assign-4.1.1.tgz";,
- "integrity": "sha1-IQmtx5ZYh8/AXLvUQsrIv7s2CGM=",
- "dev": true
+ "integrity": "sha1-IQmtx5ZYh8/AXLvUQsrIv7s2CGM="
},
"object-keys": {
"version": "1.0.11",
@@ -6064,11 +6086,11 @@
"integrity":
"sha512-eRzhrN1WSINYCDCbrz796z37LOe3m5tmW7RQf6oBntukAG1nmovJvhnwHHRMAfeoItc1m2Hk02WER2aQ/iqs+A=="
},
"query-string": {
- "version": "4.3.4",
- "resolved":
"https://registry.npmjs.org/query-string/-/query-string-4.3.4.tgz";,
- "integrity": "sha1-u7aTucqRXCMlFbIosaArYJBD2+s=",
- "dev": true,
+ "version": "5.0.1",
+ "resolved":
"https://registry.npmjs.org/query-string/-/query-string-5.0.1.tgz";,
+ "integrity":
"sha512-aM+MkQClojlNiKkO09tiN2Fv8jM/L7GWIjG2liWeKljlOdOPNWr+bW3KQ+w5V/uKprpezC7fAsAMsJtJ+2rLKA==",
"requires": {
+ "decode-uri-component": "0.2.0",
"object-assign": "4.1.1",
"strict-uri-encode": "1.1.0"
}
@@ -6936,8 +6958,7 @@
"strict-uri-encode": {
"version": "1.1.0",
"resolved":
"https://registry.npmjs.org/strict-uri-encode/-/strict-uri-encode-1.1.0.tgz";,
- "integrity": "sha1-J5siXfHVgrH1TmWt3UNS4Y+qBxM=",
- "dev": true
+ "integrity": "sha1-J5siXfHVgrH1TmWt3UNS4Y+qBxM="
},
"string-width": {
"version": "2.1.1",
diff --git a/package.json b/package.json
index 259a75e..bd3f051 100644
--- a/package.json
+++ b/package.json
@@ -51,7 +51,8 @@
"isomorphic-unfetch": "2.0.0",
"path-to-regexp": "2.1.0",
"preact": "8.2.6",
- "preact-render-to-string": "3.7.0"
+ "preact-render-to-string": "3.7.0",
+ "query-string": "5.0.1"
},
"devDependencies": {
"@types/assets-webpack-plugin": "3.5.0",
@@ -64,6 +65,7 @@
"@types/mocha": "2.2.44",
"@types/node": "8.0.47",
"@types/node-fetch": "1.6.7",
+ "@types/query-string": "5.0.1",
"@types/sinon": "4.0.0",
"@types/webpack-node-externals": "1.6.0",
"assets-webpack-plugin": "3.5.1",
@@ -112,7 +114,7 @@
},
{
"path": "dist/public/vendor.*.js",
- "maxSize": "8.2KB"
+ "maxSize": "9.6KB"
},
{
"path": "dist/public/pages/about.*.js",
diff --git a/src/client/index.tsx b/src/client/index.tsx
index 72b596d..ea10080 100644
--- a/src/client/index.tsx
+++ b/src/client/index.tsx
@@ -1,5 +1,6 @@
import { h, render } from "preact";
import newHistory from "history/createBrowserHistory";
+import { Location as HistoricalLocation } from "history";
import "wikimedia-ui-base/wikimedia-ui-base.css";
import "./index.css";
import { RouteResponse, newRouter } from "../common/router/router";
@@ -11,11 +12,8 @@
// Include preact/debug only in development for React DevTools integration.
// This check needs to be as defined with DefinePlugin in the webpack config so
// that Uglify can remove this if statement in production.
-if (process.env.NODE_ENV !== "production") {
- require("preact/debug");
-}
+if (process.env.NODE_ENV !== "production") require("preact/debug");
-const ssrData: SSRData = (window as any).__SSR_DATA__;
const history = newHistory();
const router = newRouter(routes);
const pageRoot = (_ => {
@@ -39,13 +37,13 @@
);
}
-function route(path: string) {
- router.route(path).then(renderPageRoot);
+function route(location: Location | HistoricalLocation) {
+ router.route(location.pathname, location.search).then(renderPageRoot);
}
// Observe the History
history.listen((location, action) => {
- route(location.pathname);
+ route(location);
if (action === "PUSH" || action === "REPLACE") {
// A new destination, reset the window scroll state.
@@ -56,6 +54,5 @@
// Replace the server rendered root, which does not include CSS, with a styled
// page that manages navigation with History. This enables the single page app
// experience.
-if (!ssrData.forceSSR) {
- route(location.pathname);
-}
+const ssrData: SSRData = (window as any).__SSR_DATA__;
+if (!ssrData.forceSSR) route(window.location);
diff --git a/src/common/pages/home.tsx b/src/common/pages/home.tsx
index 24b77c2..2eea50a 100644
--- a/src/common/pages/home.tsx
+++ b/src/common/pages/home.tsx
@@ -89,7 +89,10 @@
}) => (
<li>
<Link
- href={wiki.toPath({ path: { title, revision }, query })}
+ href={wiki.toPathQuery({
+ path: { title, revision },
+ query
+ })}
>
{text}
</Link>
@@ -116,7 +119,7 @@
text: string;
}) => (
<li>
- <Link href={summary.toPath({ path: { title }, query })}>
+ <Link href={summary.toPathQuery({ path: { title }, query })}>
{text}
</Link>
</li>
diff --git a/src/common/router/route.ts b/src/common/router/route.ts
index 4b64238..03394f1 100644
--- a/src/common/router/route.ts
+++ b/src/common/router/route.ts
@@ -1,4 +1,5 @@
import * as pathToRegExp from "path-to-regexp";
+import * as queryString from "query-string";
import { AnyComponent } from "preact";
import HttpResponse from "../http/http-response";
@@ -75,14 +76,21 @@
* the path does not match. An empty object is returned for matching
* NoPropsRoutes.
*/
- toParams(pathQuery: string): Params | void; // eslint-disable-line
no-use-before-define
+ toParams(path: string, query?: string): Params | void; //
eslint-disable-line no-use-before-define
/** Generates a URL path from Params. */
- toPath(params: Params): string; // eslint-disable-line no-use-before-define
+ toPath(params: Params): string;
+
+ /** Generates a URL query string from Params. */
+ toQuery(params: Params): string;
+
+ toPathQuery(params: Params): string;
}
export interface NoParamsRoute extends Route<undefined> {
- toPath(params?: undefined): string; // eslint-disable-line
no-use-before-define
+ toPath(params?: undefined): string;
+ toQuery(params?: undefined): string;
+ toPathQuery(params?: undefined): string;
}
/**
@@ -97,38 +105,23 @@
function toParams(
pathRegExp: RegExp,
paramNames: pathToRegExp.Key[],
- pathQuery: string
+ path: string,
+ query: string = ""
) {
- const matches = pathRegExp.exec(pathQuery);
+ const matches = pathRegExp.exec(path);
if (matches) {
const [, ...paramValues] = matches;
+ const queryParams = queryString.parse(query);
const params = {
path: paramNames.length ? {} as PathParams : undefined,
- query: undefined
+ query: Object.keys(queryParams).length ? queryParams : undefined
};
paramNames.forEach((param, index) => {
if (params.path) params.path[param.name] = paramValues[index];
});
- // todo: query parsing.
return params;
}
return undefined;
-}
-
-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>({
@@ -143,10 +136,20 @@
{ endsWith: "?" }
);
const pathFunction = pathToRegExp.compile(path);
+ const toPath = (params: Params) =>
+ pathFunction(params ? params.path : undefined);
+ const toQuery = (params: Params) =>
+ queryString.stringify((params && params.query) || {});
+
return {
path,
page,
- toParams: pathQuery => toParams(pathRegExp, paramNames, pathQuery),
- toPath: params => toPath(pathFunction, params)
+ toParams: (path, query) => toParams(pathRegExp, paramNames, path, query),
+ toPath,
+ toQuery,
+ toPathQuery: params => {
+ const query = toQuery(params);
+ return `${toPath(params)}${query ? `?${query}` : ""}`;
+ }
} as Route<Params>;
}
diff --git a/src/common/router/router.test.ts b/src/common/router/router.test.ts
index 2e00894..4508ce9 100644
--- a/src/common/router/router.test.ts
+++ b/src/common/router/router.test.ts
@@ -35,6 +35,28 @@
});
});
+ it("populates path and query parameters", () => {
+ const module = {
+ default: {
+ getInitialProps(params: any) {
+ assert.deepEqual(params, {
+ path: { title: "Title" },
+ query: { query: "param" }
+ });
+ return Promise.resolve({ status: 200, data: undefined });
+ },
+ Component: () => null
+ }
+ };
+ const routes = [newRoute({ path: "/wiki/:title", page: "wiki" })];
+
+ return newRouter(routes, () => Promise.resolve(module))
+ .route("/wiki/Title", "?query=param")
+ .then(rsp => {
+ assert.deepEqual(rsp.status, 200);
+ });
+ });
+
it("resolves an unknown route as a 404", () => {
return newRouter(routes, requestPageModule)
.route("/404")
diff --git a/src/common/router/router.ts b/src/common/router/router.ts
index b5fc5f4..cb3cfd8 100644
--- a/src/common/router/router.ts
+++ b/src/common/router/router.ts
@@ -1,5 +1,10 @@
import { AnyComponent } from "preact";
-import { PageComponent, PageModule, Route } from "../../common/router/route";
+import {
+ PageComponent,
+ PageModule,
+ Route,
+ RouteParams
+} from "../../common/router/route";
import HttpResponse from "../http/http-response";
import notFoundPage, { Props as NotFoundProps } from "../pages/not-found";
import errorPage, { Props as ErrorProps } from "../pages/error";
@@ -13,11 +18,11 @@
props: Props;
}
-interface RequestPageModule<Params, Props> {
- (name: string): Promise<PageModule<Params, Props>>;
+interface RequestPageModule {
+ (name: string): Promise<PageModule<Partial<RouteParams>, any>>;
}
-function getInitialProps<Params, Props>(
+function getInitialProps<Params extends Partial<RouteParams>, Props>(
module: PageComponent<Params, Props>,
params: Params
): Promise<HttpResponse<Props> | void> {
@@ -32,12 +37,14 @@
* @param {string} page The page chunk basename with no extension. Corresponds
* to Route.page.
*/
-function requestPageModuleChunk(page: string): Promise<PageModule<any, any>> {
+function requestPageModuleChunk(
+ page: string
+): Promise<PageModule<Partial<RouteParams>, any>> {
return import(/* webpackChunkName: "pages/[request]" */ `../pages/${page}`);
}
-function respond<Params, Props>(
- requestPageModule: RequestPageModule<Params, Props>,
+function respond<Params extends Partial<RouteParams>, Props>(
+ requestPageModule: RequestPageModule,
route: Route<Params>,
params: Params
): Promise<RouteResponse<Props>> {
@@ -94,12 +101,13 @@
export function newRouter(
routes: Route<any>[],
- requestPageModule: RequestPageModule<any, any> = requestPageModuleChunk
+ requestPageModule: RequestPageModule = requestPageModuleChunk
) {
return {
- route(pathQuery: string): Promise<RouteResponse<any>> {
+ route(path: string, query?: string): Promise<RouteResponse<any>> {
+ const pathQuery = `${path}${query || ""}`;
for (const route of routes) {
- const params = route.toParams(pathQuery);
+ const params = route.toParams(path, query);
if (params) {
return respond(requestPageModule, route, params).catch(error =>
respondError(pathQuery, error)
diff --git a/src/common/router/routes.test.ts b/src/common/router/routes.test.ts
index f14423e..e1ec911 100644
--- a/src/common/router/routes.test.ts
+++ b/src/common/router/routes.test.ts
@@ -8,7 +8,7 @@
randomSummary,
styleGuide
} from "./routes";
-import { PathParams, Route, RouteParams, QueryParams } from "./route";
+import { Route, RouteParams, PathParams, QueryParams } from "./route";
interface TestParams<Params extends Partial<RouteParams> | undefined> {
name: string;
@@ -18,56 +18,48 @@
// passed an unescaped string, it's worth manually assembling an unescaped
// path to test this scenario.
path: string;
+ query?: string;
params: Params;
}
-function testPathParams({ name, route, path, params }: TestParams<any>) {
+function testParams({ name, route, path, query, params }: TestParams<any>) {
it(`${name} path and route parameter types are in sync`, () => {
- const expected = {
- path: params && params.path ? {} as PathParams : undefined,
- query: params && params.query ? {} as QueryParams : undefined
+ const expected: Partial<RouteParams> = {
+ path: undefined,
+ query: undefined
};
- if (params) {
- Object.keys(params.path || {}).forEach(name => {
- const value = params.path[name];
- if (expected.path) {
- expected.path[name] =
- value === undefined ? undefined : encodeURIComponent(value);
- }
+ if (params && params.path) {
+ expected.path = {};
+ Object.keys(params.path).forEach(name => {
+ const param = params.path[name];
+ (expected.path as PathParams)[name] =
+ param === undefined ? undefined : encodeURIComponent(param);
});
-
- Object.keys(params.query || {}).forEach(name => {
- const value = params.query[name];
- if (expected.query) {
- expected.query[name] =
- value === undefined ? undefined : encodeURIComponent(value);
- }
+ }
+ if (params && params.query) {
+ // query-string objects do not have a prototype.
+ expected.query = Object.create(null);
+ Object.keys(params.query).forEach(name => {
+ const param = params.query[name];
+ (expected.query as QueryParams)[name] =
+ param === undefined ? undefined : encodeURIComponent(param);
});
}
const path = route.toPath(params);
- const result = route.toParams(path);
+ const query = route.toQuery(params);
+ const result = route.toParams(path, `?${query}`);
assert.deepStrictEqual(result, expected);
});
it(`${name} unescaped path matches`, () => {
- const expected = {
- path: params && params.path ? {} as PathParams : undefined,
- query: params && params.query ? {} as QueryParams : undefined
- };
- if (params) {
- Object.keys(params.path || {}).forEach(name => {
- const value = params.path[name];
- if (expected.path) expected.path[name] = value;
- });
-
- Object.keys(params.query || {}).forEach(name => {
- const value = params.query[name];
- if (expected.query) expected.query[name] = value;
- });
+ const expected = { path: undefined, query: undefined, ...params };
+ if (expected.query) {
+ // query-string objects do not have a prototype.
+ expected.query = Object.assign(Object.create(null), expected.query);
}
- const result = route.toParams(path);
+ const result = route.toParams(path, query ? `?${query}` : "");
assert.deepStrictEqual(result, expected);
});
}
@@ -78,6 +70,13 @@
// Note: these types are verified to be RouteParams but not verified
// to be their specific route's type.
{ name: "home", route: home, path: "/", params: undefined },
+ {
+ name: "home (global route parameters)",
+ route: home,
+ path: "/",
+ query: "query=param",
+ params: { path: undefined, query: { query: "param" } }
+ },
{ name: "about", route: about, path: "/about", params: undefined },
{
name: "wiki (title)",
@@ -233,7 +232,7 @@
path: "/dev/style-guide",
params: undefined
}
- ].forEach(testPathParams);
+ ].forEach(testParams);
});
describe("wiki", () => {
diff --git a/src/server/index.tsx b/src/server/index.tsx
index efb75bb..ce88287 100644
--- a/src/server/index.tsx
+++ b/src/server/index.tsx
@@ -1,5 +1,6 @@
import * as express from "express";
import * as compression from "compression";
+import * as queryString from "query-string";
import { h } from "preact";
import { render as renderToString } from "preact-render-to-string";
import { RouteResponse, newRouter } from "../common/router/router";
@@ -15,6 +16,10 @@
// Disable useless header.
//
http://expressjs.com/en/advanced/best-practice-security.html#at-a-minimum-disable-x-powered-by-header
server.disable("x-powered-by");
+
+// Perform manually query parsing using query-string. Request.query is now
+// always an empty object.
+server.set("query parser", false);
server.use(compression());
@@ -36,7 +41,7 @@
const router = newRouter(routes);
server.get("*", (request, response) => {
router
- .route(request.url)
+ .route(request.path, queryString.extract(request.url))
.then(routeResponse =>
response.status(routeResponse.status).send(render(routeResponse))
)
--
To view, visit https://gerrit.wikimedia.org/r/397859
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I91c5111d5b0cdda758d1f19971fe7001a052148a
Gerrit-PatchSet: 3
Gerrit-Project: marvin
Gerrit-Branch: master
Gerrit-Owner: Niedzielski <[email protected]>
Gerrit-Reviewer: Jhernandez <[email protected]>
Gerrit-Reviewer: Sniedzielski <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
