feat(sendHtml): allow custom error page #281

JamesSingleton · 2020-08-19T17:51:10Z

Description

This allows ONE_FALLBACK_URL to be given in order to define a custom
error page.

So this will require documentation on it, but I wanted to wait for approvals just in the off chance that the implementation changes due to reviews.

Motivation and Context

Allow someone to override the default error page.

How Has This Been Tested?

This was tested locally returning renderStaticErrorPage(res) and testing with different scenarios. Also, unit tests.

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (adding or updating documentation)
Dependency update

Checklist:

My change requires a change to the documentation and I have updated the documentation accordingly.
These changes should be applied to a maintenance branch.
This change requires cross browser checks.
Performance tests should be ran against the server prior to merging.
This change impacts caching for client browsers.
This change impacts HTTP headers.
This change adds additional environment variable requirements for One App users.
I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using One App?

They can now provide ONE_FALLBACK_URL to be used as the error page instead of the default.

This allows ONE_FALLBACK_URL to be given in order to define a custom error page.

oneamexbot · 2020-08-19T17:56:28Z

📊 Bundle Size Report

file name	size on disk	gzip
app.js	275.4KB	82.2KB
runtime.js	15KB	5.4KB
vendors.js	117.2KB	36.4KB
app~vendors.js	432.5KB	112.2KB
service-worker-client.js	17KB	5.2KB
legacy/app.js	290.7KB	85.4KB
legacy/runtime.js	15KB	5.4KB
legacy/vendors.js	172.7KB	47.1KB
legacy/app~vendors.js	441.3KB	114.5KB
legacy/service-worker-client.js	17.5KB	5.4KB

Generated by 🚫 dangerJS against 6317bac

Francois-Esquire · 2020-08-19T18:45:04Z

@JamesSingleton Could we add documentation for ONE_FALLBACK_URL and how we can use this feature?

JamesSingleton · 2020-08-19T18:47:01Z

@JamesSingleton Could we add documentation for ONE_FALLBACK_URL and how we can use this feature?

Yeah, I was going to wait to add the docs until after the reviews in the off chance that implementation changes based on feedback. I mentioned that in the description, I wish we could add "tasks" to be completed.

src/server/middleware/sendHtml.js

mtomcal · 2020-08-19T18:59:17Z

src/server/middleware/sendHtml.js

-                    ${message}
-                  </p>
-                </div>
+  const fallbackUrl = process.env.ONE_FALLBACK_URL;


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

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

Add ONE_FALLBACK_URL to runTime.js

src/server/middleware/sendHtml.js

__tests__/server/middleware/sendHtml.spec.js

src/server/config/env/runTime.js

nellyk · 2020-08-20T11:06:22Z

src/server/middleware/sendHtml.js

@@ -69,30 +81,36 @@ export function renderStaticErrorPage(res) {
    message = 'Sorry, we are unable to load this page at this time.';


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

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

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

can we provide, a default option?

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

__tests__/server/middleware/sendHtml.spec.js

src/server/middleware/sendHtml.js

JAdshead · 2020-08-20T16:31:54Z

src/server/middleware/sendHtml.js

@@ -69,30 +81,36 @@ export function renderStaticErrorPage(res) {
    message = 'Sorry, we are unable to load this page at this time.';


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

src/server/middleware/sendHtml.js

JAdshead

after offline discussion, warn don't throw when failing to update error page.
This is to prevent one-app failing to start when pods scale / restart due to an external factor "e.g network issue fetching error page"

__tests__/server/middleware/sendHtml.spec.js

docs/api/modules/App-Configuration.md

prod-sample/sample-modules/frank-lloyd-root/0.0.0/src/config.js

src/server/middleware/sendHtml.js

infoxicator

Apart from a couple of optionals suggestions to the name of the tests this looks really good 👍

__tests__/server/middleware/sendHtml.spec.js

__tests__/integration/one-app.spec.js

__tests__/server/middleware/sendHtml.spec.js

outdated

src/server/middleware/sendHtml.js

feat(sendHtml): allow custom error page

fc5dac6

This allows ONE_FALLBACK_URL to be given in order to define a custom error page.

JamesSingleton requested review from a team as code owners August 19, 2020 17:51

github-actions bot added the Feature label Aug 19, 2020

Merge branch 'master' into feature/custom-error-page

ca9c4b9

Merge branch 'master' into feature/custom-error-page

f5cf78f

mtomcal suggested changes Aug 19, 2020

View reviewed changes

chore(sendHtml): refactor fetchCustomErrorPage

1f7ab1f

Add ONE_FALLBACK_URL to runTime.js

mtomcal reviewed Aug 19, 2020

View reviewed changes

src/server/middleware/sendHtml.js Outdated Show resolved Hide resolved

mtomcal previously approved these changes Aug 19, 2020

View reviewed changes

infoxicator suggested changes Aug 20, 2020

View reviewed changes

src/server/middleware/sendHtml.js Outdated Show resolved Hide resolved

__tests__/server/middleware/sendHtml.spec.js Outdated Show resolved Hide resolved

nellyk reviewed Aug 20, 2020

View reviewed changes

src/server/config/env/runTime.js Outdated Show resolved Hide resolved

nellyk reviewed Aug 20, 2020

View reviewed changes

JAdshead suggested changes Aug 20, 2020

View reviewed changes

nellyk and others added 4 commits August 21, 2020 13:12

Merge branch 'master' into feature/custom-error-page

4dd32a5

Merge branch 'master' into feature/custom-error-page

ac9861e

Merge branch 'master' into feature/custom-error-page

08b889b

Merge branch 'master' into feature/custom-error-page

9a8837f

JamesSingleton marked this pull request as draft August 26, 2020 17:59

feat(errorPage): allow root module to provide error page URL

c1979aa

JamesSingleton dismissed mtomcal’s stale review via c1979aa August 27, 2020 00:20

docs(app-config): add errorPageUrl documentation

42d298b

JamesSingleton marked this pull request as ready for review August 27, 2020 19:24

JAdshead suggested changes Aug 27, 2020

View reviewed changes

src/server/middleware/sendHtml.js Outdated Show resolved Hide resolved

src/server/middleware/sendHtml.js Outdated Show resolved Hide resolved

src/server/middleware/sendHtml.js Show resolved Hide resolved

src/server/middleware/sendHtml.js Show resolved Hide resolved

James Singleton added 3 commits August 27, 2020 14:04

chore(rename): rename fetchErrorPage to setErrorPage

11474c0

Merge branch 'master' into feature/custom-error-page

d8c0913

Merge branch 'master' into feature/custom-error-page

d5c7d6c

James Singleton added 4 commits September 9, 2020 11:26

test(integration): remove extra frank-llyod-root

49d646a

Merge branch 'master' into feature/custom-error-page

9189e24

test(integration): change frank-llyod-root versions

040bd9c

chore(sendHtml): update error to a warn and update limit

65e3b64

JAdshead suggested changes Sep 10, 2020

View reviewed changes

chore(sendHtml): remove throwing errors and warn instead

353a35c

JAdshead suggested changes Sep 10, 2020

View reviewed changes

James Singleton added 3 commits September 10, 2020 12:51

chore(cleanup): updates from pr review

4c8aeb8

test(integration): update root version

2447f53

test(integration): update root version

fca02b5

infoxicator previously approved these changes Sep 11, 2020

View reviewed changes

__tests__/server/middleware/sendHtml.spec.js Outdated Show resolved Hide resolved

__tests__/integration/one-app.spec.js Outdated Show resolved Hide resolved

__tests__/server/middleware/sendHtml.spec.js Show resolved Hide resolved

test(wording): fix wording in the tests

5f79399

JamesSingleton dismissed infoxicator’s stale review via 5f79399 September 11, 2020 15:46

infoxicator previously approved these changes Sep 11, 2020

View reviewed changes

Francois-Esquire previously approved these changes Sep 11, 2020

View reviewed changes

JamesSingleton requested a review from mtomcal September 11, 2020 20:25

chore(check-status): check for a 200 status

5c5d4da

JamesSingleton dismissed stale reviews from Francois-Esquire and infoxicator via 5c5d4da September 14, 2020 20:15

Merge branch 'master' into feature/custom-error-page

99d68ad

JAdshead suggested changes Sep 15, 2020

View reviewed changes

src/server/middleware/sendHtml.js Outdated Show resolved Hide resolved

src/server/middleware/sendHtml.js Outdated Show resolved Hide resolved

chore(checks): move status check to the first check

7ae46ee

Francois-Esquire requested a review from JAdshead September 15, 2020 17:48

Francois-Esquire approved these changes Sep 15, 2020

View reviewed changes

Merge branch 'master' into feature/custom-error-page

2d8d7ac

mtomcal approved these changes Sep 15, 2020

View reviewed changes

JAdshead approved these changes Sep 15, 2020

View reviewed changes

JamesSingleton merged commit 73eb8a7 into master Sep 15, 2020

JamesSingleton deleted the feature/custom-error-page branch September 15, 2020 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sendHtml): allow custom error page #281

feat(sendHtml): allow custom error page #281

JamesSingleton commented Aug 19, 2020 •

edited

Loading

oneamexbot commented Aug 19, 2020 •

edited

Loading

Francois-Esquire commented Aug 19, 2020

JamesSingleton commented Aug 19, 2020

mtomcal Aug 19, 2020

mtomcal Aug 19, 2020

nellyk Aug 20, 2020

JamesSingleton Aug 20, 2020

JAdshead Aug 20, 2020

nellyk Aug 21, 2020

JamesSingleton Sep 3, 2020

JAdshead Aug 20, 2020

JAdshead left a comment

infoxicator left a comment

		@@ -69,30 +81,36 @@ export function renderStaticErrorPage(res) {
		message = 'Sorry, we are unable to load this page at this time.';

feat(sendHtml): allow custom error page #281

feat(sendHtml): allow custom error page #281

Conversation

JamesSingleton commented Aug 19, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of Changes

Checklist:

What is the Impact to Developers Using One App?

oneamexbot commented Aug 19, 2020 • edited Loading

📊 Bundle Size Report

Francois-Esquire commented Aug 19, 2020

JamesSingleton commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAdshead left a comment

Choose a reason for hiding this comment

infoxicator left a comment

Choose a reason for hiding this comment

JamesSingleton commented Aug 19, 2020 •

edited

Loading

oneamexbot commented Aug 19, 2020 •

edited

Loading