[DRAFT] refactor: migrated header middleware setters to fastify #807

giulianok · 2022-08-17T15:48:42Z

Description

Motivation and Context

How Has This Been Tested?

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
Security 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?

github-actions · 2022-08-17T15:52:13Z

Size Change: +7.59 kB (+1%)

Total Size: 688 kB

Filename	Size	Change
`./build/app/app.js`	173 kB	+7.5 kB (+5%)	🔍
`./build/app/app~vendors.js`	387 kB	+76 B (0%)
`./build/app/service-worker-client.js`	7.26 kB	-3 B (0%)
`./build/app/vendors.js`	114 kB	+16 B (0%)

ℹ️ View Unchanged

Filename	Size
`./build/app/runtime.js`	7.07 kB

_{compressed-size-action}

…hub.com/americanexpress/one-app into refactor/create-store-fastify-migration

github-actions · 2022-10-03T00:27:23Z

This pull request is stale because it has been open 30 days with no activity.

code-forger · 2022-11-07T16:41:47Z

src/client/initClient.jsx

@@ -37,7 +37,8 @@ export default async function initClient() {

    await loadPrerenderScripts(store.getState());

-    const { redirectLocation, renderProps } = await matchPromise({ history, routes });
+    const asd = await matchPromise({ history, routes });
+    const { redirectLocation, renderProps } = asd;


Can this be collapsed back into a one liner? or a more detailed variable name chose.

my bad, this is from testing, will remove asd

code-forger · 2022-11-07T16:46:04Z

src/server/plugins/ensureCorrelationId.js

+    }
+
+    // TODO: expose the id to the client?
+    // res.header('Correlation-Id', req.headers['correlation-id']);


Can we clear this TODO before merge, or is this a later TODO?

this got carried over and it has been in the project for ~3 years https://github.com/americanexpress/one-app/blob/main/src/server/middleware/ensureCorrelationId.js#L25-L26, I'll remove it, I don't think we need it

code-forger · 2022-11-07T16:47:42Z

src/server/plugins/reactHtml/createRequestHtmlFragment.jsx

      if (!renderProps) {
-        res.sendStatus(404);
+        // TODO: test this behaivor


have we done this now?

yes, that's covered, will remove

code-forger · 2022-11-07T16:48:25Z

src/server/plugins/reactHtml/createRequestStore.js

+      extraThunkArguments: { fetchClient },
+    });
+
+    // TODO: namespace?


Can we clear this TODO before merge?

it has been here for ~3 years https://github.com/americanexpress/one-app/blob/main/src/server/middleware/createRequestStore.js#L61, i guess we do not need it, i'll remove it

code-forger · 2022-11-07T16:48:44Z

src/server/plugins/reactHtml/createRequestStore.js

+    request.clientModuleMapCache = getClientModuleMapCache();
+  } catch (err) {
+    console.error('error creating store for request', err);
+    // TODO: migrate `renderStaticErrorPage`


Can we clear this TODO before merge?

thanks, can be removed, the next line calls the fn which has been migrated

code-forger · 2022-11-07T16:53:42Z

src/server/utils/safeRequest.js

-  'acceptsLanguages',
-  'baseUrl',
-  'forwarded',
+  // 'forwarded', // not in use (?) need to find a fastify equivalent


if this is a TODO can we mark it as such? and then, can we clear this TODO before merge?

thanks, this can be enabled again since forwardedHeaderParser was migrated already and injects the forwarded key into the request object

code-forger · 2022-11-07T16:55:29Z

prod-sample/sample-modules/frank-lloyd-root/0.0.0/src/components/Partial.jsx

+    cb();
+  } catch (error) {
+    console.error('--ERROR', error);
+  }


Nothing was meaningfully changed here right? just light refactoring?

light refactoring. The problem with react router 3 is that, if the fn throws an error, the request hangs and nothing happens. I'll revert it since this will require an extra effort to fix all the other samples and, at the end, it's just a sample

10xLaCroixDrinker · 2022-11-07T17:45:32Z

prod-sample/sample-modules/late-frank/0.0.1/src/components/LateFrank.jsx

@@ -26,6 +26,7 @@ const styles = css`
 `;

 export function FashionablyLateFrank() {
+  // eslint-disable-next-line react/no-unknown-property


Add explanations for all disabled rules

10xLaCroixDrinker · 2022-11-29T19:57:26Z

src/server/plugins/addFrameOptionsHeader.js

+    const matchedDomain = frameAncestorDomains.find((domain) => matcher.isMatch(trimmedReferrer, `${domain}/*`)
+    );


Is this for a lint issue?

10xLaCroixDrinker · 2022-11-29T20:03:05Z

src/server/plugins/reactHtml/index.jsx

+      createRequestStoreHook(request, reply, oneApp);
+
+      if (getServerPWAConfig().serviceWorker && request.url === '/_/pwa/shell') {
+        await appShell(request);


weren't we previously caching this until module map update?

10xLaCroixDrinker · 2022-11-29T20:09:33Z

src/server/utils/safeRequest.js

+  filteredRequest.originalUrl = request.raw.url;
+
+  // Not available in Fastify
+  filteredRequest.baseUrl = '';


Is this a breaking change? Seems so

10xLaCroixDrinker · 2022-11-29T20:10:56Z

__tests__/integration/one-app.spec.js

      });

+      // expect(response.status).toBe(204);


Suggested change

// expect(response.status).toBe(204);

expect(response.status).toBe(204);

10xLaCroixDrinker · 2022-11-29T20:27:04Z

__tests__/server/plugins/reactHtml/index.spec.jsx

-        }),
-        status: 200,
+
+    test('calls the expected hooks to render pwa html shell', async () => {


Suggested change

test('calls the expected hooks to render pwa html shell', async () => {

it('calls the expected hooks to render pwa html shell', async () => {

use it throughout

10xLaCroixDrinker · 2022-11-29T20:30:38Z

__tests__/server/ssrServer.spec.js

+import addFrameOptionsHeader from '../../src/server/plugins/addFrameOptionsHeader';
+import addCacheHeaders from '../../src/server/plugins/addCacheHeaders';
+import {
+  // eslint-disable-next-line import/named


add explanation

10xLaCroixDrinker · 2022-11-29T20:31:07Z

__tests__/server/ssrServer.spec.js

  beforeEach(() => {
-    jest.resetModules();
+    // jest.resetModules();


Suggested change

// jest.resetModules();

10xLaCroixDrinker · 2022-11-29T20:36:45Z

scripts/temp-fix-fastify-cors.js

+/*
+NOTE: This temporarily fix is needed until
+      this PR is reviewed, merged, and published
+      https://github.com/fastify/fastify-cors/pull/234


This PR is merged now

10xLaCroixDrinker · 2022-11-29T20:37:03Z

package.json

@@ -43,7 +43,7 @@
    "drop-module": "drop-module",
    "set-middleware": "set-middleware",
    "set-dev-endpoints": "node scripts/set-dev-endpoints.js",
-    "postinstall": "npm run build",
+    "postinstall": "node scripts/temp-fix-fastify-cors.js && npm run build",


Suggested change

"postinstall": "node scripts/temp-fix-fastify-cors.js && npm run build",

"postinstall": "npm run build",

giulianok · 2022-12-06T16:37:48Z

moved to #878

refactor: migrated header middleware setters to fastify

c558c13

github-actions bot added refactor one-app-team-review-requested labels Aug 17, 2022

giulianok and others added 9 commits August 18, 2022 16:49

refactor: migrated react to string

dee5cdf

Merge branch 'main' into refactor/create-store-fastify-migration

f745ae7

chore: fixing integration tests

9cc4d5d

fix: nounce and compression

f22f11c

chore: fix some integration tests

6682af1

Merge branch 'refactor/create-store-fastify-migration' of https://git…

f8f5da0

…hub.com/americanexpress/one-app into refactor/create-store-fastify-migration

chore: cleaning up

02cc6b7

chore: continuing fixing integration tests

a080948

chore: fixing integration tests

684e5ed

github-actions bot added the stale-pr label Oct 3, 2022

giulianok and others added 13 commits October 3, 2022 11:48

fix: pwa

0a4fd4c

chore: removed express completely from ssr server

e817bbf

chore: continue migrating to fastify

2fd3754

chore: progress

a25a939

chore: progress

d358750

chore: progress

29f2473

fix: unit tests

9ae82b7

Merge branch 'main' into refactor/create-store-fastify-migration

48c7ee2

chore: minor improvements

b131338

chore: minor adjustments

227ad1e

chore: minor fixes

d8cfdf7

chore: fixing integration tests

ca394ef

chore: temp fix for fastify cors

19e3022

giulianok marked this pull request as ready for review November 4, 2022 16:24

giulianok requested a review from a team as a code owner November 4, 2022 16:24

chore: removed unused deps

967be41

code-forger reviewed Nov 7, 2022

View reviewed changes

chore: feedback

0f2d0aa

giulianok changed the title ~~refactor: migrated header middleware setters to fastify~~ [DRAFT] refactor: migrated header middleware setters to fastify Nov 7, 2022

giulianok mentioned this pull request Nov 8, 2022

Fastify migration #852

Merged

14 tasks

Merge branch 'main' into refactor/create-store-fastify-migration

d59d71e

10xLaCroixDrinker suggested changes Nov 29, 2022

View reviewed changes

giulianok closed this Dec 6, 2022

JAdshead deleted the refactor/create-store-fastify-migration branch September 15, 2023 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] refactor: migrated header middleware setters to fastify #807

[DRAFT] refactor: migrated header middleware setters to fastify #807

giulianok commented Aug 17, 2022

github-actions bot commented Aug 17, 2022 •

edited

Loading

github-actions bot commented Oct 3, 2022

code-forger Nov 7, 2022

giulianok Nov 7, 2022

code-forger Nov 7, 2022

giulianok Nov 7, 2022

code-forger Nov 7, 2022

giulianok Nov 7, 2022

code-forger Nov 7, 2022

giulianok Nov 7, 2022

code-forger Nov 7, 2022

giulianok Nov 7, 2022

code-forger Nov 7, 2022

giulianok Nov 7, 2022

code-forger Nov 7, 2022

giulianok Nov 7, 2022

10xLaCroixDrinker Nov 7, 2022

10xLaCroixDrinker Nov 29, 2022

10xLaCroixDrinker Nov 29, 2022

10xLaCroixDrinker Nov 29, 2022

10xLaCroixDrinker Nov 29, 2022

10xLaCroixDrinker Nov 29, 2022

10xLaCroixDrinker Nov 29, 2022

10xLaCroixDrinker Nov 29, 2022

10xLaCroixDrinker Nov 29, 2022

10xLaCroixDrinker Nov 29, 2022

giulianok commented Dec 6, 2022

		const matchedDomain = frameAncestorDomains.find((domain) => matcher.isMatch(trimmedReferrer, `${domain}/*`)
		);

	// expect(response.status).toBe(204);
	expect(response.status).toBe(204);

	test('calls the expected hooks to render pwa html shell', async () => {
	it('calls the expected hooks to render pwa html shell', async () => {

	"postinstall": "node scripts/temp-fix-fastify-cors.js && npm run build",
	"postinstall": "npm run build",

[DRAFT] refactor: migrated header middleware setters to fastify #807

[DRAFT] refactor: migrated header middleware setters to fastify #807

Conversation

giulianok commented Aug 17, 2022

Description

Motivation and Context

How Has This Been Tested?

Types of Changes

Checklist:

What is the Impact to Developers Using One App?

github-actions bot commented Aug 17, 2022 • edited Loading

github-actions bot commented Oct 3, 2022

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

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

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

giulianok commented Dec 6, 2022

github-actions bot commented Aug 17, 2022 •

edited

Loading