Jhernandez has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/393226 )
Change subject: Chore: Simplify route config and deduplicate chunk naming
......................................................................
Chore: Simplify route config and deduplicate chunk naming
This patch uses a webpack feature to automatically name page chunks and
dynamically require them, thus allowing us to specify the route configs
as just the page we want with a string.
This removes the duplication between the magic comment on the route
config and the chunkname property, and encapsulates it inside the
router, which defines dynamically the chunk name in the comment and in
the property.
Changes:
* Simplify RouteConfig and remove duplication of route + chunkName by
just requiring to specify the name of the page to load from "pages/"
* Remove Props from the RouteConfig type parameters as there is no
more relation to PageModule on it
* Enhance router to import pages from pages/ (by default) based on the
route page property, and name the chunks appropriately, and then
return the chunk name to the caller based on that import
* This encapsulates the chunk naming logic and property name in the
router, instead of keeping it duplicate in the route configs
* Add a getPage argument to newRouter that encapsulates the page
fetching logic with a smart default so that we can unit test the
router
* Fix the unit tests for the router
* Other changes:
* Remove RouteParams base interface and simplify type annotations
related to route params
Change-Id: I556cdc1f2a1d4377d2d81811e928ac545fa3d085
---
M package.json
M src/common/pages/summary.tsx
M src/common/pages/wiki.tsx
M src/common/routers/api.ts
M src/common/routers/route.ts
M src/common/routers/router.test.ts
M src/common/routers/router.ts
7 files changed, 89 insertions(+), 125 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/marvin refs/changes/26/393226/1
diff --git a/package.json b/package.json
index b4c35fb..2acbd88 100644
--- a/package.json
+++ b/package.json
@@ -9,7 +9,8 @@
"watch": "run-p -s 'build:server -- -dw' start:\\* watch:test",
"build": "NODE_ENV=production run-p -s build:\\*",
"format": "npm run -s lint -- --fix",
- "lint": "eslint --cache --max-warnings 0
--report-unused-disable-directives --ext ts,tsx,js,json .",
+ "lint":
+ "eslint --cache --max-warnings 0 --report-unused-disable-directives
--ext ts,tsx,js,json .",
"test": "npm-run-all --silent -p lint build mocha -s bundlesize",
"--- SECONDARY ---": "# Useful but rarely used scripts.",
"mocha": "mocha '{src,test}/**/*.test.{ts,tsx,js}'",
@@ -18,8 +19,10 @@
"precommit": "npm test -s",
"--- INTERNAL ---": "# Private scripts.",
"start:server": "nodemon -C ${VERBOSE:--q} --watch dist/",
- "start:client": "webpack-dev-server --config src/client/webpack.config.ts
-dw",
- "watch:test": "nodemon ${VERBOSE:--q} -e js,json,ts,tsx,css --watch src/
--exec 'run-p -s lint mocha'",
+ "start:client":
+ "webpack-dev-server --config src/client/webpack.config.ts -dw",
+ "watch:test":
+ "nodemon ${VERBOSE:--q} -e js,json,ts,tsx,css --watch src/ --exec 'run-p
-s lint mocha'",
"build:server": "webpack --config src/server/webpack.config.ts",
"build:client": "webpack --config src/client/webpack.config.ts"
},
@@ -27,13 +30,7 @@
"type": "git",
"url": "git+https://phabricator.wikimedia.org/source/marvin.git";
},
- "keywords": [
- "mediawiki",
- "wikimedia",
- "web",
- "node",
- "pwa"
- ],
+ "keywords": ["mediawiki", "wikimedia", "web", "node", "pwa"],
"author": "Wikimedia Foundation",
"license": "GPL-2.0+",
"bugs": {
@@ -103,7 +100,7 @@
"bundlesize": [
{
"path": "dist/public/index.*.js",
- "maxSize": "3.2KB"
+ "maxSize": "3.4KB"
},
{
"path": "dist/public/runtime.*.js",
diff --git a/src/common/pages/summary.tsx b/src/common/pages/summary.tsx
index cac2889..fd0435f 100644
--- a/src/common/pages/summary.tsx
+++ b/src/common/pages/summary.tsx
@@ -4,7 +4,6 @@
import { PageSummary as PageSummaryModel } from "../models/page/summary";
import { PageTitleID, PageTitlePath } from "../models/page/title";
import Page from "../components/page/page";
-import { RouteParams } from "../routers/route";
import { summary } from "../routers/api";
import { request } from "../http/page-summary-http-client";
import ContentHeader from "../components/content-header/content-header";
@@ -13,7 +12,7 @@
import { RedirectError } from "../http/fetch-with-redirect";
import { unmarshalPageTitleID } from
"../marshallers/page-base/page-base-unmarshaller"; // eslint-disable-line
max-len
-interface PageParams extends RouteParams {
+interface PageParams {
/**
* When used as an input, an (unencoded, not necessarily denormalized)
* possible PageTitleID; when used as an output of Route.toParams(), an
diff --git a/src/common/pages/wiki.tsx b/src/common/pages/wiki.tsx
index 0df863c..2687fdc 100644
--- a/src/common/pages/wiki.tsx
+++ b/src/common/pages/wiki.tsx
@@ -4,7 +4,6 @@
import { Page as PageModel } from "../models/page/page";
import { PageTitleID, PageTitlePath } from "../models/page/title";
import Page from "../components/page/page";
-import { RouteParams } from "../routers/route";
import { wiki } from "../routers/api";
import { requestPage } from "../http/page-http-client";
import ContentFooter from "../components/content-footer/content-footer";
@@ -14,7 +13,7 @@
import { Thumbnail } from "../components/thumbnail/thumbnail";
import { unmarshalPageTitleID } from
"../marshallers/page-base/page-base-unmarshaller"; // eslint-disable-line
max-len
-interface PageParams extends RouteParams {
+interface PageParams {
/**
* When used as an input, an (unencoded, not necessarily denormalized)
* possible PageTitleID; when used as an output of Route.toParams(), an
diff --git a/src/common/routers/api.ts b/src/common/routers/api.ts
index 541909e..fa9c2fc 100644
--- a/src/common/routers/api.ts
+++ b/src/common/routers/api.ts
@@ -1,62 +1,47 @@
-import { Props as WikiProps, Params as WikiParams } from "../pages/wiki";
-import {
- Props as SummaryProps,
- Params as SummaryParams
-} from "../pages/summary";
+import { Params as WikiParams } from "../pages/wiki";
+import { Params as SummaryParams } from "../pages/summary";
import { AnyRoute, NoParamsRoute, Route, newRoute } from "./route";
export const home: NoParamsRoute = newRoute({
path: "/",
- importModule: () =>
- import(/* webpackChunkName: "pages/home" */ "../pages/home"),
- chunkName: "pages/home"
+ page: "home"
});
export const about: NoParamsRoute = newRoute({
path: "/about",
- importModule: () =>
- import(/* webpackChunkName: "pages/about" */ "../pages/about"),
- chunkName: "pages/about"
+ page: "about"
});
const TITLE_CHARACTER_REGEX_STRING =
"[ %!\"$&'\\(\\)*,\\-.\\/0-9:;=@A-Z\\\\^_`a-z~\\x80-\\xFF+]";
-export const wiki: Route<WikiParams, WikiProps> = newRoute({
+export const wiki: Route<WikiParams> = newRoute({
// https://www.mediawiki.org/wiki/Manual:$wgLegalTitleChars
// https://en.wikipedia.org/w/api.php?action=query&meta=siteinfo
path: `/wiki/:title(${TITLE_CHARACTER_REGEX_STRING}+?)/:revision(\\d+)?`, //
eslint-disable-line max-len
- importModule: () =>
- import(/* webpackChunkName: "pages/wiki" */ "../pages/wiki"),
- chunkName: "pages/wiki"
+ page: "wiki"
});
-export const summary: Route<SummaryParams, SummaryProps> = newRoute({
+export const summary: Route<SummaryParams> = newRoute({
// Note: title is specified as non-greedy here only to omit a trailing slash
// from the title as the wiki endpoint does.
path: `/page/summary/:title(${TITLE_CHARACTER_REGEX_STRING}+?)`,
- importModule: () =>
- import(/* webpackChunkName: "pages/summary" */ "../pages/summary"),
- chunkName: "pages/summary"
+ page: "summary"
});
-export const randomWiki: NoParamsRoute<WikiProps> = newRoute({
+export const randomWiki: NoParamsRoute = newRoute({
path: "/random/wiki",
- importModule: () => wiki.importModule(),
- chunkName: wiki.chunkName
-}) as NoParamsRoute<WikiProps>;
+ page: "wiki"
+});
-export const randomSummary: NoParamsRoute<SummaryProps> = newRoute({
+export const randomSummary: NoParamsRoute = newRoute({
path: "/random/summary",
- importModule: () => summary.importModule(),
- chunkName: summary.chunkName
-}) as NoParamsRoute<SummaryProps>;
+ page: "summary"
+});
export const styleGuide: NoParamsRoute = newRoute({
path: "/dev/style-guide",
- importModule: () =>
- import(/* webpackChunkName: "pages/style-guide" */ "../pages/style-guide"),
- chunkName: "pages/style-guide"
+ page: "style-guide"
});
export const routes: AnyRoute[] = [
diff --git a/src/common/routers/route.ts b/src/common/routers/route.ts
index 6da4e3b..33baaa4 100644
--- a/src/common/routers/route.ts
+++ b/src/common/routers/route.ts
@@ -3,30 +3,12 @@
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. Where applicable,
- * inputs may be changed by service redirects such as those for title
- * denormalization.
- *
- * Note: only strings are ever returned as outputs of Route.toParams() and all
- * inputs are converted to strings in Route.toPath().
- */
-export interface RouteParams {
- [name: string]: string | undefined;
-}
-
-/**
* A file that exposes a Preact UI component and optionally a function to
* request properties needed to construct the component. Modules within the
* pages/ subdirectory should implicitly implement this interface or typing
will
* fail in routers/api.
*/
-export type PageComponent<
- Params extends RouteParams | undefined = undefined,
- Props = undefined
-> =
+export type PageComponent<Params, Props = undefined> =
| {
/**
* A function that returns a Promise for the dependencies needed to
@@ -52,35 +34,23 @@
Component: Partial<AnyComponent<undefined, any>>;
};
-export interface PageModule<
- Params extends RouteParams | undefined = undefined,
- Props = undefined
-> {
+export interface PageModule<Params, Props = undefined> {
default: PageComponent<Params, Props>;
}
/** A plain configuration used to generate a Route. */
-export interface RouteConfig<
- Params extends RouteParams | undefined = undefined,
- Props = undefined
-> {
+export interface RouteConfig {
/**
- * A path-to-regexp URL path. This is tightly coupled to RouteParams which
- * must define a subset of properties.
+ * A path-to-regexp URL path. This is tightly coupled to Params which must
+ * define a subset of properties.
*/
path: string;
- /** Requests a PageModule. */
- importModule(): Promise<PageModule<Params, Props>>;
-
- /** The chunk filename of the module. */
- chunkName: string;
+ /** The base page to load inside pages/ */
+ page: string;
}
-export interface Route<
- Params extends RouteParams | undefined = undefined,
- Props = undefined
-> extends RouteConfig<Params, Props> {
+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.
@@ -91,12 +61,11 @@
toPath(params: Params): string;
}
-export interface NoParamsRoute<Props = undefined>
- extends Route<undefined, Props> {
+export interface NoParamsRoute extends Route<undefined> {
toPath(params?: undefined): string;
}
-export type AnyRoute = Route<any, any>;
+export type AnyRoute = Route<any>;
/**
* Decompose a URL path into a Params map for use by
@@ -119,7 +88,7 @@
const matches = pathRegExp.exec(path);
if (matches) {
const [, ...paramValues] = matches;
- const params: RouteParams = {};
+ const params: any = {};
paramNames.forEach((param, index) => {
params[param.name] = paramValues[index];
});
@@ -128,21 +97,13 @@
return undefined;
};
-export function newRoute<
- Params extends RouteParams | undefined = undefined,
- Props = undefined
->({
- path,
- importModule,
- chunkName
-}: RouteConfig<Params, Props>): Route<Params, Props> {
+export function newRoute<Params>({ path, page }: RouteConfig): Route<Params> {
const paramNames: pathToRegExp.Key[] = [];
const pathRegExp = pathToRegExp(path, paramNames);
return {
path,
- importModule,
- chunkName,
+ page,
toParams: (path: string) => toParams({ pathRegExp, paramNames, path }),
toPath: pathToRegExp.compile(path)
- } as Route<Params, Props>;
+ } as Route<Params>;
}
diff --git a/src/common/routers/router.test.ts
b/src/common/routers/router.test.ts
index be9265b..131290f 100644
--- a/src/common/routers/router.test.ts
+++ b/src/common/routers/router.test.ts
@@ -3,17 +3,17 @@
// Import the module statically to avoid potential timeouts in CI for a dynamic
// import, given ts-node compiles files when required (inside the test if using
// a dynamic import)
+// @ts-ignore
import * as HomeModule from "../pages/home";
import { RedirectError } from "../http/fetch-with-redirect";
-import { newRoute } from "./route";
+import { newRoute, PageModule } from "./route";
import { newRouter } from "./router";
const routes = [
newRoute({
path: "/",
- importModule: () => Promise.resolve(HomeModule),
- chunkName: "components/pages/home"
+ page: "home"
})
];
@@ -38,24 +38,31 @@
// eslint-disable-next-line max-len
it("throws redirect errors up for handling on the server/client
environment", done => {
- newRouter([
+ // Page module that throws a redirect
+ const page: PageModule<undefined, undefined> = {
+ default: {
+ getInitialProps() {
+ // Trick TS and eslint for tests
+ if ((_ => true)()) throw new RedirectError(301, "/redirected-url");
+ return Promise.reject("Doesn't matter");
+ },
+ Component: () => null
+ }
+ };
+
+ const getPage = (name: string) =>
+ name === "redirect"
+ ? Promise.resolve(page)
+ : Promise.reject(new Error("No page found"));
+
+ const routes = [
newRoute({
path: "/redirect",
- importModule: () =>
- Promise.resolve({
- default: {
- getInitialProps() {
- // Trick TS and eslint for tests
- if ((_ => true)())
- throw new RedirectError(301, "/redirected-url");
- return Promise.reject("Doesn't matter");
- },
- Component: () => null
- }
- }),
- chunkName: "doesnotmatter"
+ page: "redirect"
})
- ])
+ ];
+
+ newRouter(routes, getPage)
.route("/redirect")
.catch(err => {
assert.ok(
diff --git a/src/common/routers/router.ts b/src/common/routers/router.ts
index d062b80..c902ff7 100644
--- a/src/common/routers/router.ts
+++ b/src/common/routers/router.ts
@@ -3,8 +3,7 @@
AnyRoute,
PageComponent,
PageModule,
- Route,
- RouteParams
+ Route
} from "../../common/routers/route";
import HttpResponse from "../http/http-response";
@@ -19,7 +18,7 @@
props: Props;
}
-function getInitialProps<Params extends RouteParams | undefined, Props>(
+function getInitialProps<Params, Props>(
module: PageComponent<Params, Props>,
params: Params
): Promise<HttpResponse<Props> | void> {
@@ -28,16 +27,26 @@
: Promise.resolve();
}
-function respond<Params extends RouteParams | undefined, Props>(
- route: Route<Params, Props>,
+/**
+ * Imports a page module from common/pages/* and names the chunk pages/* so
that
+ * the router can tell the server the name of the chunks to preload
+ */
+function getChunk(name: string) {
+ return import(/* webpackChunkName: "pages/[request]" */ `../pages/${name}`);
+}
+
+function respond<Params, Props>(
+ getPage: PageResolver,
+ route: Route<Params>,
params: Params
): Promise<RouteResponse<Props>> {
- return route.importModule().then((module: PageModule<Params, Props>) =>
+ return getPage(route.page).then((module: PageModule<Params, Props>) =>
getInitialProps(
module.default,
params
).then((response: HttpResponse<Props> | void) => ({
- chunkName: route.chunkName,
+ // Chunk name, see {getChunk}, this variable should follow that structure
+ chunkName: `pages/${route.page}`,
status: (response && response.status) || module.default.status || 200,
Component: module.default.Component as AnyComponent<Props, any>,
props: (response && response.data) as Props
@@ -58,13 +67,20 @@
};
}
-export const newRouter = (routes: AnyRoute[]) => {
+interface PageResolver {
+ (name: string): Promise<PageModule<any, any>>;
+}
+
+export const newRouter = (
+ routes: AnyRoute[],
+ getPage: PageResolver = getChunk
+) => {
return {
route(path: string): Promise<RouteResponse<any>> {
for (const route of routes) {
const params = route.toParams(path);
if (params) {
- return respond(route, params).catch(respondError);
+ return respond(getPage, route, params).catch(respondError);
}
}
return Promise.resolve({
--
To view, visit https://gerrit.wikimedia.org/r/393226
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I556cdc1f2a1d4377d2d81811e928ac545fa3d085
Gerrit-PatchSet: 1
Gerrit-Project: marvin
Gerrit-Branch: master
Gerrit-Owner: Jhernandez <jhernan...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
