Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

feat(sendHtml): allow custom error page #281

Merged
merged 44 commits into from
Sep 15, 2020
Merged

feat(sendHtml): allow custom error page #281

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
fc5dac6
feat(sendHtml): allow custom error page
Aug 19, 2020
ca9c4b9
Merge branch 'master' into feature/custom-error-page
Aug 19, 2020
f5cf78f
Merge branch 'master' into feature/custom-error-page
Francois-Esquire Aug 19, 2020
1f7ab1f
chore(sendHtml): refactor fetchCustomErrorPage
Aug 19, 2020
4dd32a5
Merge branch 'master' into feature/custom-error-page
nellyk Aug 21, 2020
ac9861e
Merge branch 'master' into feature/custom-error-page
Aug 24, 2020
08b889b
Merge branch 'master' into feature/custom-error-page
Aug 25, 2020
9a8837f
Merge branch 'master' into feature/custom-error-page
Aug 26, 2020
c1979aa
feat(errorPage): allow root module to provide error page URL
Aug 27, 2020
42d298b
docs(app-config): add errorPageUrl documentation
Aug 27, 2020
11474c0
chore(rename): rename fetchErrorPage to setErrorPage
Aug 27, 2020
d8c0913
Merge branch 'master' into feature/custom-error-page
Aug 27, 2020
d5c7d6c
Merge branch 'master' into feature/custom-error-page
Aug 27, 2020
a49d566
Merge branch 'master' into feature/custom-error-page
Aug 28, 2020
94ec8bc
chore(timeout): add timeoutFetch to setErrorPage
Sep 1, 2020
a4f5e4c
Merge branch 'master' into feature/custom-error-page
Sep 2, 2020
6317bac
Merge branch 'master' into feature/custom-error-page
Sep 3, 2020
bb942f8
Merge branch 'master' into feature/custom-error-page
Francois-Esquire Sep 3, 2020
0437396
test(sendHtml): add expect for errorPageUrl
Sep 3, 2020
f8d6728
chore(sendHtml): move message and html template into else block
Sep 3, 2020
8a34066
Merge branch 'master' into feature/custom-error-page
JAdshead Sep 7, 2020
7f13c1f
test(integration): add error page integration test
Sep 8, 2020
2952ba3
test(integration): update integration test
Sep 8, 2020
3e40ccb
test(integration): update custom error page test
JamesSingleton Sep 8, 2020
80fbad3
test(integration): fix custom error page expect
JamesSingleton Sep 8, 2020
fdcbeb4
chore(lint): fix linting error
JamesSingleton Sep 8, 2020
0832a9d
test(integration): fix custom error page expect
JamesSingleton Sep 8, 2020
9ec77d6
Merge branch 'master' into feature/custom-error-page
Sep 8, 2020
cb31684
Merge branch 'master' into feature/custom-error-page
Sep 8, 2020
b126f42
test(integration): custom error page test
Sep 8, 2020
18c3d9e
test(integration): custom error page test
Sep 8, 2020
49d646a
test(integration): remove extra frank-llyod-root
Sep 9, 2020
9189e24
Merge branch 'master' into feature/custom-error-page
Sep 9, 2020
040bd9c
test(integration): change frank-llyod-root versions
Sep 9, 2020
65e3b64
chore(sendHtml): update error to a warn and update limit
Sep 9, 2020
353a35c
chore(sendHtml): remove throwing errors and warn instead
Sep 10, 2020
4c8aeb8
chore(cleanup): updates from pr review
Sep 10, 2020
2447f53
test(integration): update root version
Sep 10, 2020
fca02b5
test(integration): update root version
Sep 10, 2020
5f79399
test(wording): fix wording in the tests
Sep 11, 2020
5c5d4da
chore(check-status): check for a 200 status
Sep 14, 2020
99d68ad
Merge branch 'master' into feature/custom-error-page
nellyk Sep 15, 2020
7ae46ee
chore(checks): move status check to the first check
Sep 15, 2020
2d8d7ac
Merge branch 'master' into feature/custom-error-page
Sep 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 60 additions & 1 deletion __tests__/server/middleware/sendHtml.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ jest.mock('../../../src/universal/utils/transit', () => ({
}));

jest.spyOn(console, 'info').mockImplementation(() => {});
jest.spyOn(console, 'log').mockImplementation(() => {});
// jest.spyOn(console, 'log').mockImplementation(() => {});
JamesSingleton marked this conversation as resolved.
Show resolved Hide resolved
jest.spyOn(console, 'error').mockImplementation(() => {});

describe('sendHtml', () => {
Expand Down Expand Up @@ -777,6 +777,9 @@ describe('sendHtml', () => {
});

describe('renderStaticErrorPage', () => {
beforeEach(() => {
jest.clearAllMocks();
});
it('sends default static error page', () => {
renderStaticErrorPage(res);
expect(res.send).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -825,6 +828,62 @@ describe('sendHtml', () => {
expect(res.send.mock.calls[0][0]).not.toMatch('[object ');
expect(res.send.mock.calls[0][0]).not.toContain('undefined');
});

it('returns default error page if custom error page url is not given', () => {
process.env.ONE_FALLBACK_URL = '';

renderStaticErrorPage(res);

expect(res.send).toHaveBeenCalledTimes(1);
expect(res.send.mock.calls[0][0]).toContain('<!DOCTYPE html>');
expect(res.send.mock.calls[0][0]).toContain('<meta name="application-name" content="one-app">');
});

it('returns custom error page', async () => {
process.env.ONE_FALLBACK_URL = 'https://example.com';
const mockResponse = `<!doctype html>
<html>
<head>
<title>Custom Error Page</title>
<meta charset="utf-8" />
<meta http-equiv="Content-type" content="text/html; charset=utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
</head>
<body>
<div>
<h1>Custom Error Page</h1>
<p>This is a custom error page.</p>
</div>
</body>
</html>`;
global.fetch = jest.fn(() => Promise.resolve({
text: () => Promise.resolve(mockResponse),
headers: new global.Headers({
'Content-Type': 'text/html',
}),
}));

renderStaticErrorPage(res);

const data = await global.fetch.mock.results[0].value;

expect(global.fetch).toHaveBeenCalledTimes(1);
expect(global.fetch).toHaveBeenCalledWith('https://example.com');
expect(await data.text()).toBe(mockResponse);
});

it('returns default error page if response headers are not text/html', async () => {
process.env.ONE_FALLBACK_URL = 'https://example.com';
global.fetch = jest.fn(() => Promise.resolve({
headers: new global.Headers({
'Content-Type': 'text/plains',
}),
}));

await expect(renderStaticErrorPage(res)).rejects.toEqual(
new Error('Content-Type was not of type text/html')
JamesSingleton marked this conversation as resolved.
Show resolved Hide resolved
);
});
});
describe('safeSend', () => {
it('should res.send if no headers were sent', () => {
Expand Down
5 changes: 5 additions & 0 deletions src/server/config/env/runTime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,11 @@ const runTime = [
normalize: (value) => value === 'true',
defaultValue: () => false,
},
{
name: 'ONE_FALLBACK_URL',
JamesSingleton marked this conversation as resolved.
Show resolved Hide resolved
defaultValue: undefined,
validate: isFetchableUrlInNode,
},
];

runTime.forEach(preprocessEnvVar);
Expand Down
64 changes: 41 additions & 23 deletions src/server/middleware/sendHtml.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,19 @@ export function safeSend(res, ...payload) {
}
}

export function renderStaticErrorPage(res) {
export async function fetchCustomErrorPage(fallbackUrl, res) {
const response = await fetch(fallbackUrl);
JamesSingleton marked this conversation as resolved.
Show resolved Hide resolved
// If the Content-Type is not text/html throw an error
const contentType = response.headers.get('content-type');
if (!contentType.includes('text/html')) {
throw new Error('Content-Type was not of type text/html');
JamesSingleton marked this conversation as resolved.
Show resolved Hide resolved
}
// Read the response as text.
const data = await response.text();
return safeSend(res, data);
}

export async function renderStaticErrorPage(res) {
if (!res.statusCode) {
res.status(500);
}
Expand All @@ -69,30 +81,36 @@ export function renderStaticErrorPage(res) {
message = 'Sorry, we are unable to load this page at this time.';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to have this custom too? especially with internationalization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly in another PR, this PR is just to allow a custom error page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is meant to be the final catch all. we can not add anything that could risk an error being thrown here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we provide, a default option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally if they wanted a custom message they would just use a custom error page right?

}

// TODO: allow root module to provide custom error message and override default html
safeSend(res,
`<!DOCTYPE html>
<html>
<head>
<title>One App</title>
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1">
<meta name="application-name" content="one-app">
</head>
<body style="background-color: #F0F0F0">
<div id="root">
<div>
<div style="width: 70%; background-color: white; margin: 4% auto;">
<h2 style="display: flex; justify-content: center; padding: 40px 15px 0px;">Loading Error</h2>
<p style="display: flex; justify-content: center; padding: 10px 15px 40px;">
${message}
</p>
</div>
const fallbackUrl = process.env.ONE_FALLBACK_URL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, we will need to have this environment variable validated alongside the other env variables.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/americanexpress/one-app/blob/master/src/server/config/env/runTime.js


const errorResponse = `<!DOCTYPE html>
JamesSingleton marked this conversation as resolved.
Show resolved Hide resolved
<html>
<head>
<title>One App</title>
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1">
<meta name="application-name" content="one-app">
</head>
<body style="background-color: #F0F0F0">
<div id="root">
<div>
<div style="width: 70%; background-color: white; margin: 4% auto;">
<h2 style="display: flex; justify-content: center; padding: 40px 15px 0px;">Loading Error</h2>
<p style="display: flex; justify-content: center; padding: 10px 15px 40px;">
${message}
</p>
</div>
</div>
</body>
</html>`);
</div>
</body>
</html>`;

if (fallbackUrl) {
await fetchCustomErrorPage(fallbackUrl, res);
} else {
safeSend(res, errorResponse);
JamesSingleton marked this conversation as resolved.
Show resolved Hide resolved
}
}

function renderI18nScript(clientInitialState, appBundlesURLPrefix) {
Expand Down