Add withAuth HOC [SDK-2120] #189
Conversation
| } | ||
|
|
||
| public componentDidMount(): void { | ||
| window.location.assign(createLoginUrl(Router.pathname)); |
There was a problem hiding this comment.
Since useEffect runs after the render, by using a class-based component with componentDidMount we're avoiding one extra render.
src/hooks/with-auth.tsx
Outdated
| return { user: null, children: undefined }; | ||
| } | ||
|
|
||
| return { user: session.user as NullableUserProfile, children: undefined }; |
There was a problem hiding this comment.
Session uses a Claims type for the profile.
| return { user: result, children: undefined }; | ||
| } | ||
|
|
||
| const session = await instance.getSession(context.req as NextApiRequest, context.res as NextApiResponse); |
There was a problem hiding this comment.
context.req in getInitialProps is an IncomingMessage, so we need to update the types od getSession to accept IncomingMessage | NextApiRequest (and the same for ServerResponse and NextApiResponse)
Also getSession will error if context.res is undefined, so we need to figure out when that would be and handle those cases
There was a problem hiding this comment.
In next they're also casting, so it seems safe. But it's probably better to use a union type.
There was a problem hiding this comment.
Also getSession will error if context.res is undefined, so we need to figure out when that would be and handle those cases
I'll look into that 👍🏼
There was a problem hiding this comment.
Just tried the union type: Argument of type 'ServerResponse | undefined' is not assignable to parameter of type 'ServerResponse | NextApiResponse<any>'. Will have to keep the cast.
| export default function withAuth( | ||
| InnerComponent: React.ElementType, | ||
| redirect: () => React.ReactElement, | ||
| instance: ISignInWithAuth0 |
There was a problem hiding this comment.
I wonder if it would be better for ISignInWithAuth0 to have a withAuth wrapper like the handlers (rather than having to pass the instance in) eg
// profile-ssr.jsx
import { withAuth } from '../lib/auth0';There was a problem hiding this comment.
Good idea, so the the instance would be providing the HOC (and it would already have the instance 'closed over').
|
|
||
| export default function withAuth( | ||
| InnerComponent: React.ElementType, | ||
| redirect: () => React.ReactElement, |
There was a problem hiding this comment.
Can we go for an options object here fro the other params? (like https://github.com/auth0/auth0-react/blob/master/src/with-authentication-required.tsx#L75)
It gives us more flexibility when designing the API
There was a problem hiding this comment.
Yup, I'll change it.
There was a problem hiding this comment.
But if the instance will be providing the HOC, as in :
// profile-ssr.jsx
import { withAuth } from '../lib/auth0';does it still make sense to use an options object? Because it will only be two params now (the inner component, and the render prop).
| return <InnerComponent {...props} user={user} />; | ||
| }; | ||
|
|
||
| Authenticated.getInitialProps = async (context: NextPageContext): Promise<AuthenticatedProps> => { |
There was a problem hiding this comment.
Also, since we're targeting 9.4+ we should probably use getServerSideProps
If you're using Next.js 9.3 or newer, we recommend that you use getStaticProps or getServerSideProps instead of getInitialProps.
https://nextjs.org/docs/api-reference/data-fetching/getInitialProps
| }; | ||
|
|
||
| Authenticated.getInitialProps = async (context: NextPageContext): Promise<AuthenticatedProps> => { | ||
| if (!context.req) { |
There was a problem hiding this comment.
I'm not sure about this either, context.req will be falsey when rendering the static page at build time, so we shouldn't be checking the session here - let's have a chat about this, because I'm not sure how this should work at the moment
Description
This PR adds the
withAuthfrontend HOC, that allows to fetch the user profile on server-side rendered pages.Testing
Checklist
master