-
Notifications
You must be signed in to change notification settings - Fork 87
Conversation
feat: migrated express to fastify
Size Change: -2.02 kB (0%) Total Size: 682 kB
ℹ️ View Unchanged
|
Just fyi, the bundles are identical, not sure why this diff is showing that, for example, app.js got an increased of 5% Fastify feature branchReport from
|
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.
Hold on merging for discussion on testing
This pull request is stale because it has been open 30 days with no activity. |
feat(fastifycdn): migrate holocron cdn to fastify
9a8c17e
fix: matching headers with existing express output
@@ -115,11 +119,13 @@ | |||
"joi": "^17.6.0", | |||
"lean-intl": "^4.2.2", | |||
"matcher": "^4.0.0", | |||
"node-fetch": "^2.6.7", |
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.
we already have cross-fetch installed
"node-fetch": "^2.6.7", |
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.
node-fetch
was added in this PR https://github.com/americanexpress/one-app/pull/882/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519. @dogpatch626 do you know if we need it installed and that particular version?
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.
yea we use that particular version in multiple repos for testing in particular I believe. installing it was just to replace got with node-fetch.
package.json
Outdated
@@ -165,6 +172,7 @@ | |||
"eslint-plugin-jest": "^24.7.0", | |||
"eslint-plugin-jest-dom": "^3.9.4", | |||
"expect": "^28.1.1", | |||
"express": "^4.18.2", |
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.
Why is this here?
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.
right, completely forgot, those are left overs, we do not need them anymore. Also found some others that I just removed
@@ -20,6 +20,7 @@ import { jsx, css } from '@emotion/core'; | |||
import styles from './styles.scss'; | |||
|
|||
const HelloMessage = () => ( | |||
// eslint-disable-next-line react/no-unknown-property |
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.
Can you update the lint config for the prod-sample so we dont need this?
renderProps: { | ||
routes, | ||
components: [() => 'hi'], | ||
// history: browserHistory, |
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.
// history: browserHistory, |
}); | ||
|
||
describe('empty frame-ancestors', () => { | ||
test('does not add frame options header', () => { |
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.
in the addCacheHeaders.spec.js
test file we use it
, and here we are using test
. We should be consistent
ExpressJS to Fastify migration. This is an internal change, API isn't being impacted.
Description
Migrating away from ExpressJS to improve performance, essentially be able to reduce time-to-start resolving a request, time-to-response, and handle more requests per second.
Small PRs have been raised for the team to easily review the changes:
#854
#856
#857
#859
#860
#865
This PR is the result of all these small PRs
Metrics
Fastify benchmark
Fastify runner + ExpressJS routes & logic (current implementation)
ExpressJS only
This is just to provide some context of the Fastify improvement so far
Motivation and Context
It improves requests performance
How Has This Been Tested?
Types of Changes
Checklist:
What is the Impact to Developers Using One App?