Skip to content

feat(auth): Next.js App Router support #2106

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

Merged
merged 24 commits into from
Jan 25, 2024
Merged

feat(auth): Next.js App Router support #2106

merged 24 commits into from
Jan 25, 2024

Conversation

hsynlms
Copy link
Contributor

@hsynlms hsynlms commented Dec 20, 2023

This feature brings Next.js App Router support to the Thirdweb Auth package and fixes #2022.

Problem solved

Thirdweb Auth Next generates API handlers for Next.js Pages Router apps. But it is still not fully compatible with the new Next.js App Router.

Next.js App Router has become stable since v13.4.

In this PR, I kept existing router support which is Pages Router and added compatibility with App Router.

Example usage

// file: /app/api/auth/[...thirdweb]/route.ts

// Next.js - Pages Router
import { ThirdwebAuth } from '@thirdweb-dev/auth/next'
import { PrivateKeyWallet } from '@thirdweb-dev/auth/evm'

export const { ThirdwebAuthHandler, getUser } = ThirdwebAuth({
  domain: '<your_app_domain>',
  wallet: new PrivateKeyWallet('<your_wallet_private_key>'),
})

export {
  ThirdwebAuthHandler as GET,
  ThirdwebAuthHandler as POST
}

// Next.js - App Router
import { ThirdwebAuthAppRouter } from '@thirdweb-dev/auth/next'
import { PrivateKeyWallet } from '@thirdweb-dev/auth/evm'

export const { ThirdwebAuthHandler, getUser } = ThirdwebAuthAppRouter({
  domain: '<your_app_domain>',
  wallet: new PrivateKeyWallet('<your_wallet_private_key>'),
})

export {
  ThirdwebAuthHandler as GET,
  ThirdwebAuthHandler as POST
}

Changes made

  • Public API changes: list the public API changes made if any
  • Internal API changes: explain the internal logic changes

How to test

  • Automated tests: link to unit test file
  • Manual tests: You are now able to use Next.js App Router route handler for Thirdweb Auth.

This feature brings Next.js App Router support to the Thirdweb Auth package and fixes #2022.
@hsynlms hsynlms requested a review from a team December 20, 2023 12:43
Copy link

changeset-bot bot commented Dec 20, 2023

🦋 Changeset detected

Latest commit: 14ba584

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

This PR includes changesets to release 7 packages
Name Type
@thirdweb-dev/auth Patch
thirdweb Patch
@thirdweb-dev/react-core Patch
@thirdweb-dev/react Patch
@thirdweb-dev/unity-js-bridge Patch
@thirdweb-dev/react-native Patch
@thirdweb-dev/react-native-compat 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

Copy link

socket-security bot commented Dec 20, 2023

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/[email protected]

View full report↗︎

@hsynlms
Copy link
Contributor Author

hsynlms commented Dec 20, 2023

Btw, I also brought skipFetchSetup development here (#2085) because whenever it's missing or it's value is false, the RPC provider throws below error:

Error: could not detect network (event="noNetwork", code=NETWORK_ERROR, version=providers/5.7.2)
     at Logger.makeError (webpack-internal:///(rsc)/../../node_modules/.pnpm/@[email protected]/node_modules/@ethersproject/logger/lib.esm/index.js:240:23)
     at Logger.throwError (webpack-internal:///(rsc)/../../node_modules/.pnpm/@[email protected]/node_modules/@ethersproject/logger/lib.esm/index.js:249:20)
     at JsonRpcProvider.eval (webpack-internal:///(rsc)/../../node_modules/.pnpm/@[email protected]/node_modules/@ethersproject/providers/lib.esm/json-rpc-provider.js:513:27)
     at Generator.throw (<anonymous>)
     at rejected (webpack-internal:///(rsc)/../../node_modules/.pnpm/@[email protected]/node_modules/@ethersproject/providers/lib.esm/json-rpc-provider.js:34:40)
     at runNextTicks (node:internal/process/task_queues:60:5)
     at listOnTimeout (node:internal/timers:540:9)
     at process.processTimers (node:internal/timers:514:7) {
   reason: 'could not detect network',
   code: 'NETWORK_ERROR',
   event: 'noNetwork'
 }

@hsynlms
Copy link
Contributor Author

hsynlms commented Dec 20, 2023

The below document and code example need to be updated accordingly.
https://portal.thirdweb.com/auth/server-frameworks/next
https://github.com/thirdweb-example/thirdweb-auth-next

The function might be used/called in a Next.js component or elsewhere. So it's not always a must to pass a request object. This is a breaking change.
@joaquim-verges
Copy link
Member

Thanks for the PR! We'll take a look shortly

Copy link
Contributor

@adam-maj adam-maj left a comment

Choose a reason for hiding this comment

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

Thanks a lot for putting up this PR, super helpful!

As far as approach - rather than replacing the support for auth in pages directory with app directory as it is here, we should support both pages (old code) and app (new code) since there are users still using both.

The best path here would likely just to create separate exports for each of them - ie the old export could be called ThirdwebAuthPages and the new one could be called ThirdwebAuthApp or something like ThirdwebAuth and ThirdwebAuthRouter, etc.

So essentially, would just add back the old code and change the export name, and then keep your new code as a separate folder with another export.

Feel free to make this change - or I'll get to it after my current focus (might take a few days/week+)

@hsynlms
Copy link
Contributor Author

hsynlms commented Jan 8, 2024

@adam-maj sure. I will try to find a way to do it 'cause I really need this feature asap :)

domain: ctx.cookieOptions?.domain,
path: ctx.cookieOptions?.path || "/",
sameSite: ctx.cookieOptions?.sameSite || "none",
expires: new Date(Date.now() + 5 * 1000),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adam-maj I wonder why setting the cookie expiry to 5 seconds instead of deleting the cookie at that time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adam-maj ping

@hsynlms
Copy link
Contributor Author

hsynlms commented Jan 9, 2024

@adam-maj There are a few boring things to cover Pages and App Routers support in the same package. For more information see the below reasons list.

I suggest that it'd be better to create a new package (next-legacy) for Pages Router. So the next package will only be compatible with App Router and the below list will make no sense then.

  • The only way to have the request object in App Router is route handlers. So most functions require the request object but there is no need. For example in the getUser and ThirdwebAuthConfig callback functions.
  • We need to put a lot of TODOs to describe the things that need to be done when Pages Router is deprecated.
  • It seems hard to give support for both routers in the same ThirdwebAuthHandler function. Because the returned async function signature varies with each router.
  • Changing function names by adding suffixes (like ThirdwebAuthPages, ThirdwebAuthApp, etc.,) makes things harder to maintain and I think is not good for the developer experience.

@adam-maj
Copy link
Contributor

@adam-maj There are a few boring things to cover Pages and App Routers support in the same package. For more information see the below reasons list.

I suggest that it'd be better to create a new package (next-legacy) for Pages Router. So the next package will only be compatible with App Router and the below list will make no sense then.

  • The only way to have the request object in App Router is route handlers. So most functions require the request object but there is no need. For example in the getUser and ThirdwebAuthConfig callback functions.
  • We need to put a lot of TODOs to describe the things that need to be done when Pages Router is deprecated.
  • It seems hard to give support for both routers in the same ThirdwebAuthHandler function. Because the returned async function signature varies with each router.
  • Changing function names by adding suffixes (like ThirdwebAuthPages, ThirdwebAuthApp, etc.,) makes things harder to maintain and I think is not good for the developer experience.

Thanks for the update & comment! Given that many (from what I've seen, most) Next + Auth users are still using the pages directory because there are many people that haven't migrated over/haven't learned app router, I don't want to deprecate the pages directory support - as both are fully valid in Next 13 itself.

Instead of handling the same routes in one ThirdwebAuthHandler, I'd suggest just having 2 separate handler functions - like ThirdwebAuth and ThirdwebAuthAppRouter (this being the non-default).

You should be able to keep ThirdwebAuth exactly the same as before. Then ThirdwebAuthAppRouter can change the implementation in all the ways you describe above to make it conducive to the /app router format. These can both ship from the same package, and we'll be sure to document it so it's clear what to use when.

With that setup, should be able to merge in the PR and get it all build ASAP

@hsynlms hsynlms requested a review from adam-maj January 12, 2024 12:14
@hsynlms
Copy link
Contributor Author

hsynlms commented Jan 22, 2024

@adam-maj did you review the last commit?

@adam-maj
Copy link
Contributor

@hsynlms Thanks - this looks perfect! Have you tested both the router and pages versions? If not I can help with this, an I'll merge ASAP right after.

@hsynlms
Copy link
Contributor Author

hsynlms commented Jan 22, 2024

@adam-maj I rebuilt the package and tested it with my App Router app. It works! But let's double check and ensure it works properly. So I need your tests too :)

@adam-maj
Copy link
Contributor

/release-pr

@adam-maj
Copy link
Contributor

Btw @hsynlms one question - what's the reasoning behind the abstract wallets change in packages/wallets/src/evm/wallets/abstract.ts - was this intentional?

Signed-off-by: Adam Majmudar <[email protected]>
@adam-maj
Copy link
Contributor

/release-pr

@adam-maj
Copy link
Contributor

@hsynlms Just tested for both app router and pages and works like a charm! Pending merge conflict resolution and the last question, should be good to go and I'll merge 😄

@hsynlms
Copy link
Contributor Author

hsynlms commented Jan 24, 2024

@hsynlms Just tested for both app router and pages and works like a charm! Pending merge conflict resolution and the last question, should be good to go and I'll merge 😄

Sure! See the below comment. That's why I edited the packages/wallets/src/evm/wallets/abstract.ts file.
#2106 (comment)

@hsynlms hsynlms requested review from jnsdls and a team as code owners January 24, 2024 08:10
@hsynlms hsynlms requested a review from a team January 24, 2024 08:10
@hsynlms
Copy link
Contributor Author

hsynlms commented Jan 24, 2024

@adam-maj I think the PR is ready to go!

@adam-maj
Copy link
Contributor

@hsynlms Perfect, good to go - thanks for putting this together!

@adam-maj adam-maj enabled auto-merge January 24, 2024 20:11
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6884815) 67.51% compared to head (14ba584) 67.08%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2106      +/-   ##
==========================================
- Coverage   67.51%   67.08%   -0.44%     
==========================================
  Files         291      291              
  Lines       11016    11016              
  Branches     1513     1513              
==========================================
- Hits         7438     7390      -48     
- Misses       2952     3001      +49     
+ Partials      626      625       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adam-maj adam-maj added this pull request to the merge queue Jan 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 24, 2024
@adam-maj adam-maj added this pull request to the merge queue Jan 25, 2024
Merged via the queue into thirdweb-dev:main with commit 1a5d41c Jan 25, 2024
@jnsdls jnsdls mentioned this pull request Jan 23, 2024
@hsynlms hsynlms deleted the feat-next-app-router-support branch January 25, 2024 09:31
jnsdls pushed a commit that referenced this pull request Jun 19, 2024
Bumps [react-hook-form](https://github.com/react-hook-form/react-hook-form) from 7.48.2 to 7.49.3.
- [Release notes](https://github.com/react-hook-form/react-hook-form/releases)
- [Changelog](https://github.com/react-hook-form/react-hook-form/blob/master/CHANGELOG.md)
- [Commits](react-hook-form/react-hook-form@v7.48.2...v7.49.3)

---
updated-dependencies:
- dependency-name: react-hook-form
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Nacho Iacovino <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When will Thirdweb give support for Next.js App Router?
3 participants