-
Notifications
You must be signed in to change notification settings - Fork 86
Conversation
BREAKING CHANGE: Upgrade from React 16 to 17
BREAKING CHANGE: minimum supported node version is 16
* chore(babel): update packages * chore(commitlint): update * chore(rollup-plugins): update * chore(acorn): uninstall * chore(babel-preset-amex): update to 4 * chore(body-parser): update * chore(dev-deps): update * chore(holocron): update 1.3.0 * chore(redux): update 4.2.0 * chore(core-js): update 3.23.3 * chore(deps): run npm update * chore(husky): update to 8.x * chore(chalk): downgrade to non esm version * chore(webdriverio): update 7.x * feat(dockerfile): update node version to 16.15.1 * chore(deps): update supertest * fix(node): set min version 16.15.1 * chore(deps): dedupe * test(createRequestHtmlFragment): more reliable error message * chore(jest): upgrade 28.1.2
BREAKING CHANGE: Upgrade from React 16 to 17
BREAKING CHANGE: minimum supported node version is 16
* chore(babel): update packages * chore(commitlint): update * chore(rollup-plugins): update * chore(acorn): uninstall * chore(babel-preset-amex): update to 4 * chore(body-parser): update * chore(dev-deps): update * chore(holocron): update 1.3.0 * chore(redux): update 4.2.0 * chore(core-js): update 3.23.3 * chore(deps): run npm update * chore(husky): update to 8.x * chore(chalk): downgrade to non esm version * chore(webdriverio): update 7.x * feat(dockerfile): update node version to 16.15.1 * chore(deps): update supertest * fix(node): set min version 16.15.1 * chore(deps): dedupe * test(createRequestHtmlFragment): more reliable error message * chore(jest): upgrade 28.1.2
Size Change: 0 B Total Size: 681 kB ℹ️ View Unchanged
|
CHANGELOG.md
Outdated
* **deps:** upgrade to react 17 ([9ba5c39](https://github.com/americanexpress/one-app/commit/9ba5c3926f603a8bf3e8a4ed32930766568621eb)) | ||
* **server:** drop node 12 support ([5154d65](https://github.com/americanexpress/one-app/commit/5154d65924dec28f63ad5fdf9192c4ecb972d440)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be included
@@ -75,7 +75,7 @@ const setUpTestRunner = async ({ | |||
dockerComposeUpProcess.stderr.pipe(logWatcherDuplex); | |||
|
|||
// uncomment this line in order to view full logs for debugging | |||
// logWatcherDuplex.pipe(process.stdout); | |||
logWatcherDuplex.pipe(process.stdout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logWatcherDuplex.pipe(process.stdout); | |
// logWatcherDuplex.pipe(process.stdout); |
src/server/middleware/sendHtml.js
Outdated
@@ -327,6 +327,7 @@ export default function sendHtml(req, res) { | |||
|
|||
console.info(`sendHtml, have store? ${!!store}, have appHtml? ${!!appHtml}`); | |||
if (appHtml && typeof appHtml !== 'string') { | |||
console.log('--test 1', 'before throw'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove console.log
s
console.log('--test 1', 'before throw'); |
src/server/middleware/sendHtml.js
Outdated
@@ -345,10 +346,12 @@ export default function sendHtml(req, res) { | |||
const allowedHtmlTags = clientInitialState.getIn(['rendering', 'renderTextOnlyOptions', 'allowedHtmlTags']); | |||
|
|||
if (renderPartialOnly) { | |||
console.log('--test 2', 'renderPartialOnly'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log('--test 2', 'renderPartialOnly'); |
src/server/middleware/sendHtml.js
Outdated
return safeSend(res, renderPartial({ html: req.appHtml, store, disableStyles })); | ||
} | ||
|
||
if (renderTextOnly) { | ||
console.log('--test 3', 'renderTextOnly'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log('--test 3', 'renderTextOnly'); |
src/server/middleware/sendHtml.js
Outdated
@@ -390,9 +393,11 @@ export default function sendHtml(req, res) { | |||
</html> | |||
`; | |||
} catch (err) { | |||
console.log('--test 4', 'ERROR'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log('--test 4', 'ERROR'); |
src/server/middleware/sendHtml.js
Outdated
console.error('sendHtml had an error, sending static error page', err); | ||
return renderStaticErrorPage(res); | ||
} | ||
|
||
console.log('--test 5', 'safeSend'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log('--test 5', 'safeSend'); |
: {}; | ||
}; | ||
|
||
export const listen = async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you planning on moving back to ./listen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not since this is a fn only consumed in index.js
@@ -0,0 +1,107 @@ | |||
/* | |||
* Copyright 2019 American Express Travel Related Services Company, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Copyright 2019 American Express Travel Related Services Company, Inc. | |
* Copyright 2022 American Express Travel Related Services Company, Inc. |
|
||
app.use(serverError); | ||
// TODO: Needs refactoring (priority) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this TODO for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is for when we migrate the express routes to fastify. Server errors are being handled from the express router but we still need a 404 handler on the top level
Co-authored-by: Jamie King <jamie.king@aexp.com>
__tests__/server/ssrServer.spec.js
Outdated
@@ -1,3 +1,4 @@ | |||
/* eslint-disable jest/no-disabled-tests */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* eslint-disable jest/no-disabled-tests */ |
.get('/route'); | ||
expect(addFrameOptionsHeader).toBeCalled(); | ||
}); | ||
|
||
it('should return the static error page when an error was encountered', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still being tested elsewhere ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's covered by fastify but I added it back
Express to Fastify migration that will improve overall performance on requests/responses including SSR
Description
The goal with this PR is to run all One App servers under Fastify while supporting existing ExpressJS code to slowly migrate to Fastify. This is done through a library called
@fastify/express
that allows us run ExpressJS through Fastify.Motivation and Context
Fastify handles more requests per second than ExpressJS. It is also better maintained, provides a cleaner syntax, encapsulation, and more. To minimize risk, in this PR, we are introducing Fastify + Express as described in the Description section above.
Benchmark from Fastify website
Benchmark local with fastify-express
The metrics still show that fastify handles 4x the amount of requests compared to express, and fastify-express is slower than fastify but still 3x faster than express
How Has This Been Tested?
Tests were performed via manual tests, unit testing, and integration tests with v5 and v6 versions.
Types of Changes
Checklist:
What is the Impact to Developers Using One App?
There's no impact in DX at the moment