fix(core): add cache-control header to cognito identity client #10753
Conversation
packages/core/src/Credentials.ts
Outdated
| @@ -296,6 +297,16 @@ export class CredentialsClass { | |||
| customUserAgent: getAmplifyUserAgent(), | |||
| }); | |||
|
|
|||
| cognitoClient.middlewareStack.add( | |||
There was a problem hiding this comment.
Is there ever a case when we would not want the cache control header to be added? If not, could we just bake this into the CognitoClient constructor?
There was a problem hiding this comment.
The answer to your question is 'no'; and while we don't control the client constructor itself (it's an sdk construct) we've moved instantiation up into a separate utility that bakes in this middleware.
packages/core/src/Credentials.ts
Outdated
| @@ -296,6 +297,16 @@ export class CredentialsClass { | |||
| customUserAgent: getAmplifyUserAgent(), | |||
| }); | |||
|
|
|||
| cognitoClient.middlewareStack.add( | |||
| (next, _) => (args: any) => { | |||
| args.request.headers['cache-control'] = 'no-store'; | |||
There was a problem hiding this comment.
Nit: Might want to do this immutably to keep the middleware side-effect free.
const updatedArgs = {
...args,
request: {
...args.request,
headers: {
...args.request.headers,
'cache-control': 'no-store'
}
}
};
Also might be worth breaking into a utility function to improve re-usability & test-ability?
haverchuck
force-pushed
the
adds-cache-control-header
branch
from
15d86be
to
a3ef771
Compare
Codecov Report
@@ Coverage Diff @@
## main #10753 +/- ##
==========================================
- Coverage 86.16% 86.16% -0.01%
==========================================
Files 196 197 +1
Lines 18354 18368 +14
Branches 3906 3906
==========================================
+ Hits 15815 15826 +11
- Misses 2464 2468 +4
+ Partials 75 74 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
What do you think of having an E2E for checking the headers instead of doing it on the unit tests? I would try to test the logic on unit tests but test the actual requests headers I would do it on cypress on a real app.
Thanks @haverchuck 🎖️
| expect(cognitoClient).toBeTruthy(); | ||
| }); | ||
|
|
||
| test('headers should be added by middleware on GetIdCommand', async () => { |
There was a problem hiding this comment.
I would add expect.assertions and also use done in case the tests ends before doing the assertions
There was a problem hiding this comment.
I don't think the done is necessary if you are using async callback and awaiting, but I added the expect.assertions. Good callout, thanks.
haverchuck
dismissed stale reviews from cshfang, stocaaro, and AllanZhengYP
via
f199384
haverchuck
dismissed stale reviews from stocaaro, jimblanc, AllanZhengYP, and cshfang
via
f25109b
Description of changes
Adds the cache-control 'no-store' header via middleware to the various instantiations of the Cognito Identity Pool Client.
Please note: the
Credentialsfile instantiates the cognito client multiple times - most likely needlessly. I've left this in place, though, to minimize the scope of the change.Description of how you validated changes
Manual testing.
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.