Niedzielski has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/392093 )
Change subject: Update: show 404 page when content cannot be found
......................................................................
Update: show 404 page when content cannot be found
• Detect and throw an Error when fetch status indicates failure.
fetch-with-redirect should probably be renamed as it's now a generic
HTTP client abstraction.
• Show the not found page for any 4xx HTTP status error. This prevents a
confusing raw error log from being presented to the user for simple
page not found scenarios. The logic for determining when to show this
page lives in the router to prevent duplication in both the server and
client.
• Statically bundle the not-found page since this will become a generic
error page. api.ts just defines chunkName as an empty string now and
the typing may be improved later.
• The vendor chunk changed because it now includes unfetch. It isn't
clear why this library wasn't already in there since there's no
minimum on the number of children for it be bundled.
Bug: T177000
Change-Id: Icac4b44a6e6c29af9823fc73e13087463715e248
---
M package.json
M src/common/http/fetch-with-redirect.ts
M src/common/routers/api.ts
M src/common/routers/router.ts
M src/server/index.tsx
5 files changed, 73 insertions(+), 34 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/marvin refs/changes/93/392093/1
diff --git a/package.json b/package.json
index 1d5d82c..2dadc3c 100644
--- a/package.json
+++ b/package.json
@@ -103,15 +103,15 @@
"bundlesize": [
{
"path": "dist/public/index.*.js",
- "maxSize": "2.8KB"
+ "maxSize": "3.2KB"
},
{
"path": "dist/public/runtime.*.js",
- "maxSize": "1.1KB"
+ "maxSize": "1KB"
},
{
"path": "dist/public/vendor.*.js",
- "maxSize": "7.8KB"
+ "maxSize": "8.2KB"
},
{
"path": "dist/public/pages/about.*.js",
@@ -122,16 +122,12 @@
"maxSize": "0.8KB"
},
{
- "path": "dist/public/pages/not-found.*.js",
- "maxSize": "0.3KB"
- },
- {
"path": "dist/public/pages/summary.*.js",
- "maxSize": "2.9KB"
+ "maxSize": "2.2KB"
},
{
"path": "dist/public/pages/wiki.*.js",
- "maxSize": "3.3KB"
+ "maxSize": "2.6KB"
}
]
}
diff --git a/src/common/http/fetch-with-redirect.ts
b/src/common/http/fetch-with-redirect.ts
index afa4730..b4f3ce4 100644
--- a/src/common/http/fetch-with-redirect.ts
+++ b/src/common/http/fetch-with-redirect.ts
@@ -1,14 +1,12 @@
+// todo: rename this module.
+
import * as unfetch from "isomorphic-unfetch";
/** true if executing on server, false otherwise. */
const server = typeof window !== "object";
-/** Server-only redirect status code and destination URL. */
-export class RedirectError extends Error {
- /** Status code in the domain of [300, 399]. */
+export class FetchError extends Error {
status: number;
-
- /** Destination URL. */
url: string;
constructor(status: number, url: string) {
@@ -17,6 +15,15 @@
this.url = url;
}
}
+
+/** Server-only 3xx redirect status code and destination URL. */
+export class RedirectError extends FetchError {}
+
+/** 4xx status code errors. */
+export class ClientError extends FetchError {}
+
+/** 5xx status code errors. */
+export class ServerError extends FetchError {}
/**
* Isomorphic fetch with transparent throw-on-redirect behavior for requests
@@ -37,6 +44,13 @@
const url = response.headers.get("location");
throw new RedirectError(response.status, url as string);
}
+ const url = typeof input === "string" ? input : input.url;
+ if (response.status >= 400 && response.status <= 499) {
+ throw new ClientError(response.status, url);
+ }
+ if (response.status >= 500) {
+ throw new ServerError(response.status, url);
+ }
return response;
});
}
diff --git a/src/common/routers/api.ts b/src/common/routers/api.ts
index 5484e5f..7222b4a 100644
--- a/src/common/routers/api.ts
+++ b/src/common/routers/api.ts
@@ -1,4 +1,4 @@
-import { Params as NotFoundParams } from "../pages/not-found";
+import * as notFoundModule from "../pages/not-found";
import { Props as WikiProps, Params as WikiParams } from "../pages/wiki";
import {
Props as SummaryProps,
@@ -60,13 +60,12 @@
chunkName: "pages/style-guide"
});
-export const notFound: Route<NotFoundParams> = newRoute({
+export const notFound: Route<notFoundModule.Params> = newRoute({
// `(.*)` is the new `*`. See
// https://github.com/pillarjs/path-to-regexp/issues/37.
path: "(.*)",
- importModule: () =>
- import(/* webpackChunkName: "pages/not-found" */ "../pages/not-found"),
- chunkName: "pages/not-found"
+ importModule: () => Promise.resolve(notFoundModule),
+ chunkName: ""
});
export const routes: AnyRoute[] = [
diff --git a/src/common/routers/router.ts b/src/common/routers/router.ts
index 5f488e4..d216737 100644
--- a/src/common/routers/router.ts
+++ b/src/common/routers/router.ts
@@ -7,6 +7,10 @@
RouteParams
} from "../../common/routers/route";
import HttpResponse from "../http/http-response";
+import { PRODUCTION } from "../assets/config";
+import { ServerError, ClientError } from "../http/fetch-with-redirect";
+import * as notFoundModule from "../pages/not-found";
+import { notFound } from "./api";
export interface RouteResponse<Props> {
chunkName: string;
@@ -28,17 +32,42 @@
route: Route<Params, Props>,
params: Params
): Promise<RouteResponse<Props>> {
- return route.importModule().then((module: PageModule<Params, Props>) =>
- getInitialProps(
- module.default,
- params
- ).then((response: HttpResponse<Props> | void) => ({
- chunkName: route.chunkName,
- status: (response && response.status) || module.default.status || 200,
- Component: module.default.Component as AnyComponent<Props, any>,
- props: (response && response.data) as Props
- }))
- );
+ return route
+ .importModule()
+ .then((module: PageModule<Params, Props>) =>
+ getInitialProps(
+ module.default,
+ params
+ ).then((response: HttpResponse<Props> | void) => ({
+ chunkName: route.chunkName,
+ status: (response && response.status) || module.default.status || 200,
+ Component: module.default.Component as AnyComponent<Props, any>,
+ props: (response && response.data) as Props
+ }))
+ )
+ .catch(error => {
+ const message = `${error.message}\n${error.stack}`;
+ if (error instanceof TypeError) {
+ // todo: show poor connectivity page on client.
+ } else if (error instanceof ClientError) {
+ if (!PRODUCTION) console.log(message); // eslint-disable-line
no-console
+ // todo: return response.status (T177000).
+ return {
+ chunkName: notFound.chunkName,
+ status: error.status,
+ Component: notFoundModule.default.Component,
+ props: {} as Props
+ };
+ } else if (error instanceof ServerError) {
+ // todo: show 5xx page and log for Services / RI and
+ // return response.status.
+ console.error(message); // eslint-disable-line no-console
+ } else {
+ // todo: show 5xx page and log for Marvin.
+ console.error(message); // eslint-disable-line no-console
+ }
+ throw error;
+ });
}
export const newRouter = (routes: AnyRoute[]) => {
diff --git a/src/server/index.tsx b/src/server/index.tsx
index 3e46ff2..f8d923c 100644
--- a/src/server/index.tsx
+++ b/src/server/index.tsx
@@ -38,11 +38,12 @@
.status(error.status)
.header("location", error.url)
.send();
- } else {
- const message = `${error.message}\n${error.stack}`;
- console.error(message); // eslint-disable-line no-console
- return response.status(500).send(message);
}
+
+ // todo: show 5xx page and log for Marvin.
+ const message = `${error.message}\n${error.stack}`;
+ console.error(message); // eslint-disable-line no-console
+ return response.status(500).send(message);
});
});
--
To view, visit https://gerrit.wikimedia.org/r/392093
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icac4b44a6e6c29af9823fc73e13087463715e248
Gerrit-PatchSet: 1
Gerrit-Project: marvin
Gerrit-Branch: master
Gerrit-Owner: Niedzielski <sniedziel...@wikimedia.org>
Gerrit-Reviewer: Sniedzielski <sniedziel...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
