-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Next.js SSR Improvements #877
base: master
Are you sure you want to change the base?
Conversation
944a484
to
e278f9f
Compare
size-limit report 📦
|
f2af627
to
b306f85
Compare
b306f85
to
ad89bbe
Compare
110ee20
to
8231792
Compare
8231792
to
7c23d35
Compare
// TODO: This method is isolated atm to make it easier to test and debug | ||
// In the end it should be merged in the getSSRSession function |
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 do we need to merge it into getSSRSession? At first glance this works well as both the usecase and the impl is different enough.
const redirectTo = "/"; | ||
|
||
const authPagePath = `${getAuthPagePath()}`; | ||
const refreshLocation = `/api/auth/session/refresh?redirectTo=${redirectTo}`; |
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.
getRefreshLocation
?
// TODO: | ||
// - Do we need to check the token version and handle ERR_JWKS_MULTIPLE_MATCHING_KEYS like in the node SDK? |
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.
I don't think we do, that only comes up with really old tokens IIRC
): Promise<{ isValid: true; payload: AccessTokenPayload["up"] } | { isValid: false }> { | ||
const appInfo = SuperTokensNextjsSSRAPIWrapper.getConfigOrThrow().appInfo; | ||
const jwksUrl = new URL(`${appInfo.apiBasePath}/jwt/jwks.json`, appInfo.apiDomain); | ||
const JWKS = jose.createRemoteJWKSet(jwksUrl); |
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.
This should be cached/moved into a global location.
logDebugMessage("Missing tokens from refresh response"); | ||
return redirectToAuthPage(request); | ||
} | ||
const redirectUrl = new URL(redirectTo, request.url); |
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.
Do we check if redirectTo is relative?
// getSetCookie was added in node 18 and our build target is ES5 | ||
// This should not a problem here since the function runs in the Vercel edge runtime environment | ||
// @ts-expect-error TS(2339): Property 'getSetCookie' does not exist on type 'Headers'. |
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.
As a TODO: let's review if we could raise our build-target
@@ -113,3 +113,5 @@ export function validateAndCompareOnFailureRedirectionURLToCurrent(redirectURL: | |||
|
|||
return currentUrl === fullRedirectURL; | |||
} | |||
|
|||
export function getSessionForSSR() {} |
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.
I think this can be removed.
@@ -75,3 +75,28 @@ export type AccessDeniedThemeProps = { | |||
export type ComponentOverrideMap = { | |||
SessionAccessDenied_Override?: ComponentOverride<typeof AccessDeniedScreenTheme>; | |||
}; | |||
|
|||
export type AccessTokenPayload = { |
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.
I think this can be removed.
@@ -0,0 +1,154 @@ | |||
import * as jose from "jose"; |
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.
I think this can be removed.
@@ -0,0 +1,205 @@ | |||
import * as jose from "jose"; |
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.
You might want to remove this or move it into the with-typescript folder (that's what we use to "test" typings in ci/scripts as well as a scratch file)
* SessionAuthForNextJS will handle proper redirection for the user based on the different session states. | ||
* It will redirect to the login page if the session does not exist etc. |
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.
I'd say it handles redirections on the client side.
Summary of change
The following PR improves SSR support in Next.js applications.
To get the session data in the context of server side rendering you now have to call
getSSRSession
either inside aServer Component
, or ingetServerSideProps
(for older next applications).There's two parts to this change:
Fetching the session during SSR
The logic for this is kept in the
ssr.ts
file. The function reads the active tokens and has 3 possible outcomes:Refreshing a session
The
getSSRSession
function redirects to arefresh
endpoint that we do not expose from our existing API.Hence, this change adds logic inside the next.js middleware to handle such a request. The custom middleware calls the backend API using a fetch and updates the response with the new tokens.
This part is now kept mostly in the
middleware.ts
file, in the react library. It's there just to make it easy to review during this stage. It should be moved in the node SDK package as a separate, Nextjs specific API.Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)
Documentation changes
(If relevant, please create a PR in our docs repo, or create a checklist here highlighting the necessary changes)
Checklist for important updates
frontendDriverInterfaceSupported.json
file has been updated (if needed)package.json
package-lock.json
lib/ts/version.ts
npm run build-pretty
git tag
) in the formatvX.Y.Z
, and then find the latest branch (git branch --all
) whoseX.Y
is greater than the latest released tag.someFunc: function () {..}
).size-limit
section ofpackage.json
with the size limit set to the current size rounded up.rollup.config.mjs
lib/ts/types.ts
lib/ts/recipe/multifactorauth/types.ts
Remaining TODOs for this PR