This repository has been archived by the owner on May 3, 2024. It is now read-only.
EspressJS to Fastify migration #785
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
![github-advanced-security[bot]](https://github.com/avatars/in/57789?s=60&v=4){kind=small}
JAdshead
suggested changes
Jul 26, 2022
CHANGELOG.md
Outdated
Comment on lines
59
to
60
| * **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.
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.
Suggested change
| 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.
remove console.logs
Suggested change
| 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.
Suggested change
| 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.
Suggested change
| 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.
Suggested change
| 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.
Suggested change
| console.log('--test 5', 'safeSend'); |
| : {}; | ||
| }; | ||
|
|
||
| export const listen = async ({ |
There was a problem hiding this comment.
are you planning on moving back to ./listen
There was a problem hiding this comment.
I am not since this is a fn only consumed in index.js
10xLaCroixDrinker
suggested changes
Aug 4, 2022
| @@ -0,0 +1,107 @@ | |||
| /* | |||
| * Copyright 2019 American Express Travel Related Services Company, Inc. | |||
There was a problem hiding this comment.
Suggested change
| * 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.
Is this TODO for this PR?
There was a problem hiding this comment.
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>
JAdshead
reviewed
Aug 9, 2022
__tests__/server/ssrServer.spec.js
Outdated
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable jest/no-disabled-tests */ | |||
There was a problem hiding this comment.
Suggested change
| /* eslint-disable jest/no-disabled-tests */ |
JAdshead
reviewed
Aug 9, 2022
| .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.
is this still being tested elsewhere ?
There was a problem hiding this comment.
it's covered by fastify but I added it back
JAdshead
approved these changes
Aug 10, 2022
10xLaCroixDrinker
approved these changes
Aug 10, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/expressthat 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