Skip to content

fix(nextjs): Replace router.refresh() with cookies().delete()#3518

Merged
nikosdouvlis merged 3 commits intomainfrom
nikos/replace-router-refresh
Jun 11, 2024
Merged

fix(nextjs): Replace router.refresh() with cookies().delete()#3518
nikosdouvlis merged 3 commits intomainfrom
nikos/replace-router-refresh

Conversation

@nikosdouvlis
Copy link
Copy Markdown
Member

@nikosdouvlis nikosdouvlis commented Jun 5, 2024

Description

In this PR we replace router.refresh with a server action that calls cookies().delete. According to the NextJS docs, using the cookies().delete API invalidates the FE client cache, similar to what router.refresh does.

The main difference between the two is that router.refresh will then attempt to fetch and re-mount the current page. This causes our components to unmount and mount again, losing state before setActive completes. This usually leads to navigation/redirect issues, issues completing oauth flows etc.

Caveat:
We can only use server action directly for next@14 and above, as server actions were an experimental feature in next@13.
For next@13 we still call router.refresh. Users that are on next@13 should upgrade to at least next@14 if they notice any of the issues described above.

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:

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 5, 2024

🦋 Changeset detected

Latest commit: df967c2

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

@nikosdouvlis nikosdouvlis reopened this Jun 5, 2024
@LekoArts LekoArts marked this pull request as draft June 6, 2024 09:42
@joysamaddar
Copy link
Copy Markdown

joysamaddar commented Jun 7, 2024

Hi @nikosdouvlis,
Hope you are having a good day.
Is this PR related to fixing the issue where in NextJS, the protected page gets server reloaded once we press signout and hence clerk is not redirecting to the sign out route instead reloads the same route which redirects us to the signin page with the fallback of the current route?

@desiprisg
Copy link
Copy Markdown
Contributor

!snapshot

@clerk-cookie
Copy link
Copy Markdown
Collaborator

Hey @desiprisg - the snapshot version command generated the following package versions:

Package Version
@clerk/backend 1.2.2-snapshot.v958fab4
@clerk/chrome-extension 1.0.17-snapshot.v958fab4
@clerk/clerk-js 5.6.0-snapshot.v958fab4
@clerk/elements 0.6.0-snapshot.v958fab4
@clerk/clerk-expo 1.2.0-snapshot.v958fab4
@clerk/express 0.0.11-snapshot.v958fab4
@clerk/fastify 1.0.13-snapshot.v958fab4
gatsby-plugin-clerk 5.0.0-beta.45
@clerk/localizations 2.4.4-snapshot.v958fab4
@clerk/nextjs 5.1.4-snapshot.v958fab4
@clerk/clerk-react 5.2.3-snapshot.v958fab4
@clerk/remix 4.1.0-snapshot.v958fab4
@clerk/clerk-sdk-node 5.0.10-snapshot.v958fab4
@clerk/shared 2.2.2-snapshot.v958fab4
@clerk/testing 1.1.6-snapshot.v958fab4
@clerk/themes 2.1.9-snapshot.v958fab4
@clerk/types 4.6.0-snapshot.v958fab4

Tip: Use the snippet copy button below to quickly install the required packages.
@clerk/backend

npm i @clerk/backend@1.2.2-snapshot.v958fab4 --save-exact

@clerk/chrome-extension

npm i @clerk/chrome-extension@1.0.17-snapshot.v958fab4 --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@5.6.0-snapshot.v958fab4 --save-exact

@clerk/elements

npm i @clerk/elements@0.6.0-snapshot.v958fab4 --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@1.2.0-snapshot.v958fab4 --save-exact

@clerk/express

npm i @clerk/express@0.0.11-snapshot.v958fab4 --save-exact

@clerk/fastify

npm i @clerk/fastify@1.0.13-snapshot.v958fab4 --save-exact

gatsby-plugin-clerk

npm i gatsby-plugin-clerk@5.0.0-beta.45 --save-exact

@clerk/localizations

npm i @clerk/localizations@2.4.4-snapshot.v958fab4 --save-exact

@clerk/nextjs

npm i @clerk/nextjs@5.1.4-snapshot.v958fab4 --save-exact

@clerk/clerk-react

npm i @clerk/clerk-react@5.2.3-snapshot.v958fab4 --save-exact

@clerk/remix

npm i @clerk/remix@4.1.0-snapshot.v958fab4 --save-exact

@clerk/clerk-sdk-node

npm i @clerk/clerk-sdk-node@5.0.10-snapshot.v958fab4 --save-exact

@clerk/shared

npm i @clerk/shared@2.2.2-snapshot.v958fab4 --save-exact

@clerk/testing

npm i @clerk/testing@1.1.6-snapshot.v958fab4 --save-exact

@clerk/themes

npm i @clerk/themes@2.1.9-snapshot.v958fab4 --save-exact

@clerk/types

npm i @clerk/types@4.6.0-snapshot.v958fab4 --save-exact

@nikosdouvlis
Copy link
Copy Markdown
Member Author

Hi @nikosdouvlis, Hope you are having a good day. Is this PR related to fixing the issue where in NextJS, the protected page gets server reloaded once we press signout and hence clerk is not redirecting to the sign out route instead reloads the same route which redirects us to the signin page with the fallback of the current route?

Hello @joysamaddar , I hope you're having a great day as well :)
Even though I am not completely sure yet, as this fix was intended to solve a different issue, the underlying root cause does look like it's the same, so I'm fairly confident that this will resolve the issue you describe. That being said, more testing needs to be done on our end!

Would you mind sharing a GH issue that describes your problem in detail, if there is one? Happy to take a look.

@joysamaddar
Copy link
Copy Markdown

joysamaddar commented Jun 8, 2024

Hi @nikosdouvlis, Hope you are having a good day. Is this PR related to fixing the issue where in NextJS, the protected page gets server reloaded once we press signout and hence clerk is not redirecting to the sign out route instead reloads the same route which redirects us to the signin page with the fallback of the current route?

Hello @joysamaddar , I hope you're having a great day as well :) Even though I am not completely sure yet, as this fix was intended to solve a different issue, the underlying root cause does look like it's the same, so I'm fairly confident that this will resolve the issue you describe. That being said, more testing needs to be done on our end!

Would you mind sharing a GH issue that describes your problem in detail, if there is one? Happy to take a look.

Hello again,
I tried out the snapshot version of this PR and it seems this PR fixed the issue i described.
Do you still want me to make an issue for this? Happy to do so if it helps.

@nikosdouvlis nikosdouvlis marked this pull request as ready for review June 11, 2024 07:41
Comment thread packages/nextjs/src/app-router/client/ClerkProvider.tsx Outdated
@desiprisg desiprisg force-pushed the nikos/replace-router-refresh branch from ab98df2 to f17124a Compare June 11, 2024 12:53
@nikosdouvlis nikosdouvlis self-assigned this Jun 11, 2024
Copy link
Copy Markdown
Member

@anagstef anagstef left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Copy Markdown
Contributor

@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.

🚢

Copy link
Copy Markdown
Contributor

@desiprisg desiprisg left a comment

Choose a reason for hiding this comment

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

Good job on that conditional check

@nikosdouvlis nikosdouvlis merged commit 8741003 into main Jun 11, 2024
@nikosdouvlis nikosdouvlis deleted the nikos/replace-router-refresh branch June 11, 2024 14:55
This was referenced Jun 11, 2024
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.

7 participants