feat(adapter-nextjs): add createAuthRouteHandlers to createServerRunner #13801
Conversation
HuiSF
requested review from
ukhan-amazon,
haverchuck,
cshfang and
jimblanc
as code owners
HuiSF
force-pushed
the
hui/feat/adapter-nextjs/2-auth-api-handlers
branch
from
f934d7a
to
8e12c20
Compare
HuiSF
changed the title
HuiSF
force-pushed
the
hui/feat/adapter-nextjs/2-auth-api-handlers
branch
3 times, most recently
from
afba807
to
25c4eaa
Compare
HuiSF
force-pushed
the
hui/feat/adapter-nextjs/apply-cookie-attrs
branch
from
6cbdad1
to
af39e86
Compare
HuiSF
force-pushed
the
hui/feat/adapter-nextjs/2-auth-api-handlers
branch
from
25c4eaa
to
26ec9ed
Compare
HuiSF
force-pushed
the
hui/feat/adapter-nextjs/apply-cookie-attrs
branch
from
af39e86
to
fa5d86c
Compare
HuiSF
force-pushed
the
hui/feat/adapter-nextjs/2-auth-api-handlers
branch
from
26ec9ed
to
67d073f
Compare
| // The `query` property is a convenience method added to the underlying `IncomingMessage`. | ||
| return ( | ||
| 'query' in request && | ||
| Object.prototype.toString.call(request.query) === '[object Object]' |
There was a problem hiding this comment.
Any particular reason to deviate from the typesafe object check on 22-23?
| config: resourcesConfig, | ||
| runtimeOptions = {}, | ||
| }: CreateAuthRouteHandlersFactoryInput): CreateAuthRouteHandlers => { | ||
| const origin = process.env.AMPLIFY_APP_ORIGIN; |
There was a problem hiding this comment.
Did you consider adding this as an input param in order to keep this function pure?
| Cognito: { | ||
| loginWith: { oauth: oAuthConfig }, | ||
| }, | ||
| } = resourcesConfig.Auth; |
There was a problem hiding this comment.
Might just be me, but nested destructuring assignment beyond the first level is hard to read 😵💫
const {
Cognito: {
loginWith: { oauth: oAuthConfig },
},
} = resourcesConfig.Auth;
vs.
const { oauth: oAuthConfig } = resourcesConfig.Auth.Cognito.loginWith;
| ['sign-up', true], | ||
| ['sign-out', true], | ||
| ['sign-out-callback', true], | ||
| ['fancy-route', false], |
| path?: string, | ||
| ): path is SupportedRoutePaths { | ||
| return ( | ||
| path !== undefined && |
There was a problem hiding this comment.
Nit: Maybe you don't even need this check since undefined wouldn't be included in supported paths
There was a problem hiding this comment.
ts doesn't like supportedRoutePaths.includes(undefined) as undefined is a not allow element type in SupportedRoutePaths
| @@ -0,0 +1,53 @@ | |||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
Nit: Maybe these should be called predicates since they don't actually assert?
| export function isAuthRoutesHandlersContext( | ||
| context: object, | ||
| ): context is AuthRoutesHandlerContext { | ||
| return ( |
There was a problem hiding this comment.
Why do some of these have such exhaustive checks but others don't? This check might technically allow for arrays, which is not what we want?
| */ | ||
| customState?: string; | ||
| /** | ||
| * The app route redirect to when a sign-in flow completes. Defaults o the |
There was a problem hiding this comment.
Nit:
| * The app route redirect to when a sign-in flow completes. Defaults o the | |
| * The app route redirect to when a sign-in flow completes. Defaults to the |
| */ | ||
| redirectOnSignInComplete?: string; | ||
| /** | ||
| * The app route redirect to when a sign-out flow completes. Defaults o the |
There was a problem hiding this comment.
Nit:
| * The app route redirect to when a sign-out flow completes. Defaults o the | |
| * The app route redirect to when a sign-out flow completes. Defaults to the |
| * The handler function for handling the GET requests sent to the Auth API routes. | ||
| * Only `GET` method gets handled, otherwise it rejects. | ||
| * | ||
| * @param request - `request` can be he following: |
There was a problem hiding this comment.
Nit:
| * @param request - `request` can be he following: | |
| * @param request - `request` can be the following: |
| import { isSupportedAuthApiRoutePath } from '../../../src/auth/utils/isSupportedAuthApiRoutePath'; | ||
|
|
||
| describe('isSupportedAuthApiRoutePath', () => { | ||
| test.each([ |
There was a problem hiding this comment.
Nit: Do you need/want to test for the undefined case?
| const mockEnd = jest.fn(); | ||
| const mockStatus = jest.fn(() => ({ end: mockEnd })); |
There was a problem hiding this comment.
Nit: These look like they can be extracted and just reused with mock clears? Would help reduce some boilerplate per test to help readability
| [ | ||
| { | ||
| params: {}, | ||
| }, | ||
| false, | ||
| ], | ||
| [ | ||
| { | ||
| params: { | ||
| slug: 'sign-in', | ||
| }, | ||
| }, | ||
| true, | ||
| ], |
There was a problem hiding this comment.
Nit: Could these really not fit on single lines? This is kinda rough to read
Description of changes
Issue #, if available
Description of how you validated changes
Checklist
yarn testpassesChecklist for repo maintainers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.