Skip to content
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

fix(nextjs): Use dynamic imports in ClerkProvider #2292

Merged

Conversation

LekoArts
Copy link
Member

@LekoArts LekoArts commented Dec 8, 2023

Description

Currently users of @clerk/nextjs and Next.js 12 (or older) are seeing an error when booting up their app:

error - ./node_modules/@clerk/nextjs/dist/esm/app-router/client/ClerkProvider.js:3:0
Module not found: Can't resolve 'next/navigation'

Import trace for requested module:
./node_modules/@clerk/nextjs/dist/esm/client-boundary/ClerkProvider.js
./node_modules/@clerk/nextjs/dist/esm/components.client.js
./node_modules/@clerk/nextjs/dist/esm/index.js
<path-to-where-they-mount-clerk-provider>

From what I can tell the error must have existed since #1840 was merged. But we only recently got a report about this so hopefully not too many people where impacted 🤞

This only needs to be fixed for v4 (as v5 will drop Next.js 12), so that's why the target branch is release/v4.

Docs PR: clerk/clerk-docs#543

Root cause

The packages/nextjs/src/client-boundary/ClerkProvider.tsx component imports next/compat/router and the ClerkProvider for the App router version of Next.js (which uses next/navigation).

Both imports only got introduced in Next.js 13 so that's why they fail on older versions. webpack tries to resolve these imports, then it fails.

Solution

Dynamic imports! Check if next/compat/router can be required, if not we're on Next.js 12 or older and we can use the ClerkProvider for Pages.

This works on the client-side, but unfortunately not during compilation of the middleware and other parts of Next.js. So we need to tell our users to let webpack ignore these imports as their code paths won't be hit.

Testing

You can test this change yourself by:

  1. Clone https://github.com/clerk/clerk-nextjs-pages-quickstart
  2. Replace the middleware with
    import { withClerkMiddleware } from "@clerk/nextjs/server";
    import { NextResponse } from "next/server";
    import type { NextRequest } from "next/server";
    
    export default withClerkMiddleware((req: NextRequest) => {
      return NextResponse.next();
    });
    
    export const config = {
      matcher: ['/((?!.+\\.[\\w]+$|_next).*)', '/', '/(api|trpc)(.*)'],
    };
  3. Replace next.config.js with
    const webpack = require('webpack');
    
    /** @type {import('next').NextConfig} */
    const nextConfig = {
      reactStrictMode: true,
      webpack(config) {
        config.plugins.push(new webpack.IgnorePlugin({ resourceRegExp: /^next\/(navigation|headers|compat\/router)$/ }))
        return config;
      }
    }
    
    module.exports = nextConfig
  4. Install packages from this PR, e.g. by using secco

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/backend
  • @clerk/chrome-extension
  • @clerk/clerk-js
  • @clerk/clerk-expo
  • @clerk/fastify
  • gatsby-plugin-clerk
  • @clerk/localizations
  • @clerk/nextjs
  • @clerk/clerk-react
  • @clerk/remix
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/themes
  • @clerk/types
  • build/tooling/chore

Copy link

changeset-bot bot commented Dec 8, 2023

🦋 Changeset detected

Latest commit: d864e20

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clerk/nextjs Patch

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

@LekoArts LekoArts marked this pull request as ready for review December 11, 2023 10:32
@LekoArts LekoArts requested a review from a team as a code owner December 11, 2023 10:32
Copy link
Member

@panteliselef panteliselef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, it's great that u added a docs page as well.

@LekoArts LekoArts added this pull request to the merge queue Dec 12, 2023
Merged via the queue into release/v4 with commit ada4bd0 Dec 12, 2023
7 checks passed
@LekoArts LekoArts deleted the lekoarts/sdk-907-fix-support-for-nextjs-below-v13 branch December 12, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants