-
Couldn't load subscription status.
- Fork 402
feat(nextjs): Hint correct middleware location when missing clerkMiddleware #4979
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(nextjs): Hint correct middleware location when missing clerkMiddleware #4979
Conversation
🦋 Changeset detectedLatest commit: 47d3f31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
|
||
| throw new Error( | ||
| `Clerk: auth() and currentUser() are only supported in App Router (/app directory).\nIf you're using /pages, try getAuth() instead.\nOriginal error: ${e}`, | ||
| `Clerk: auth(), currentUser() and clerkClient(), are only supported in App Router (/app directory).\nIf you're using /pages, try getAuth() instead.\nOriginal error: ${e}`, |
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.
Our relatively new clerkClient() also depends on 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.
Good catch
| export const createGetAuthAsync = ({ | ||
| debugLoggerName, | ||
| noAuthStatusMessage, | ||
| }: { | ||
| debugLoggerName: string; | ||
| noAuthStatusMessage: string; | ||
| }) => | ||
| createGetAuth({ | ||
| debugLoggerName: debugLoggerName, | ||
| transformer: async ({ request, options, logger }) => { | ||
| if (!detectClerkMiddleware(request)) { | ||
| // Keep the same behaviour for versions that may have issues with bundling `node:fs` | ||
| if (isNextWithUnstableServerActions) { | ||
| assertAuthStatus(request, noAuthStatusMessage); | ||
| } | ||
|
|
||
| const missConfiguredMiddlewareLocation = await import('./keyless-node.js') | ||
| .then(m => m.suggestMiddlewareLocation()) | ||
| .catch(() => undefined); | ||
|
|
||
| if (missConfiguredMiddlewareLocation) { | ||
| throw new Error(missConfiguredMiddlewareLocation); |
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.
So the refactor was necessary for using await import() without breaking the synchronous getAuth() for pages router.
From my previous experience with keyless using require() would not prevent issues when accessing node:fs
To remind folks we've hit issues with Next 13 and 14 where node:fs would leak to the browser, when code was accessing the module from a server action, causing the app to break.
…rror-message-when-middlewarets-is-mis
…e-when-middlewarets-is-mis
| }); | ||
|
|
||
| export const getAuth = createGetAuth({ | ||
| export const createGetAuthAsync = ({ |
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 it really makes sense to rename methods based on async unless we are going to support both versions sync/async. Do we need both?
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.
getAuth is a publicly consumed api for the pages router. making it async will be a breaking change.
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.
The code looks good. 👍🏼
As a team, I think we should talk about how to go forward with more async things creeping into the SDK and this duality of sync/async will become more and more problematic.
…rror-message-when-middlewarets-is-mis
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.
|
@alexcarpenter I cannot reproduce, are you sure the error in your terminal are not from before ? |
|
!snapshot |
|
Hey @alexcarpenter - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/astro@2.1.16-snapshot.v20250124164546 --save-exact
npm i @clerk/backend@1.23.8-snapshot.v20250124164546 --save-exact
npm i @clerk/chrome-extension@2.2.2-snapshot.v20250124164546 --save-exact
npm i @clerk/clerk-js@5.49.1-snapshot.v20250124164546 --save-exact
npm i @clerk/elements@0.22.16-snapshot.v20250124164546 --save-exact
npm i @clerk/clerk-expo@2.7.0-snapshot.v20250124164546 --save-exact
npm i @clerk/expo-passkeys@0.1.15-snapshot.v20250124164546 --save-exact
npm i @clerk/express@1.3.43-snapshot.v20250124164546 --save-exact
npm i @clerk/fastify@2.1.16-snapshot.v20250124164546 --save-exact
npm i @clerk/localizations@3.10.1-snapshot.v20250124164546 --save-exact
npm i @clerk/nextjs@6.10.3-snapshot.v20250124164546 --save-exact
npm i @clerk/nuxt@1.0.12-snapshot.v20250124164546 --save-exact
npm i @clerk/clerk-react@5.22.7-snapshot.v20250124164546 --save-exact
npm i @clerk/react-router@1.0.2-snapshot.v20250124164546 --save-exact
npm i @clerk/remix@4.4.18-snapshot.v20250124164546 --save-exact
npm i @clerk/shared@2.20.15-snapshot.v20250124164546 --save-exact
npm i @clerk/tanstack-start@0.8.17-snapshot.v20250124164546 --save-exact
npm i @clerk/testing@1.4.16-snapshot.v20250124164546 --save-exact
npm i @clerk/themes@2.2.13-snapshot.v20250124164546 --save-exact
npm i @clerk/types@4.44.1-snapshot.v20250124164546 --save-exact
npm i @clerk/ui@0.3.17-snapshot.v20250124164546 --save-exact
npm i @clerk/vue@1.1.7-snapshot.v20250124164546 --save-exact |
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.
@panteliselef good job identifying that so many people hit this (~33% of new installs!!!). This is an easy fix but the impact is huge in this case 👏🏻
|
|
||
| throw new Error( | ||
| `Clerk: auth() and currentUser() are only supported in App Router (/app directory).\nIf you're using /pages, try getAuth() instead.\nOriginal error: ${e}`, | ||
| `Clerk: auth(), currentUser() and clerkClient(), are only supported in App Router (/app directory).\nIf you're using /pages, try getAuth() instead.\nOriginal error: ${e}`, |
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.
Good catch
| const { existsSync } = safeNodeRuntimeFs(); | ||
| const path = safeNodeRuntimePath(); | ||
|
|
||
| const projectWithAppSrc = path.join(process.cwd(), 'src', 'app'); |
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.
Minor, but I would probably write this one like:
| const projectWithAppSrc = path.join(process.cwd(), 'src', 'app'); | |
| const projectWithAppSrc = existsSync(path.join(process.cwd(), 'src', 'app')); |
simply because projectWithAppSrc sounds like a boolean. Feel free to ignore of course
| // Utility type to determine if a type is a Promise | ||
| type IsPromise<T> = T extends Promise<any> ? true : false; | ||
|
|
||
| const createGetAuth = < |
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.
Good job building the conditional types used here.
It looks like the majority of the logic is with each transformer function. Would it be simpler to drop the transformer abstraction altogether, drop createGetAuth and end up with 2 methods:
createSyncGetAuthcreateAsyncGetAuth
(notice I moved the 'sync'/'async' adjective before the 'getAuth' part, which is a minor detail as the adjective describes the returned function and not the builder.
Duplicating a few lines would probably be worth it if we could remove an abstraction layer. What do you think?
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.
Great improvement! 🚀
Co-authored-by: Stefanos Anagnostou <anagstef@users.noreply.github.com> Co-authored-by: Nikos Douvlis <nikosdouvlis@gmail.com>
…e-when-middlewarets-is-mis

Description
When the middleware.ts file is mislocated, Clerk will throw a related error as
clerkMiddewareneeds to run for the server-side helpers to work.This PR aims to improve the error by showing actionable steps, as we identified that 1 out of 3 users will misplace the middleware file when using Next for the first time.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change