Skip to content

Update useAwaitableNavigate to handle navigations between pages reliably#2889

Closed
nikosdouvlis wants to merge 2 commits intorelease/v4from
nikos/fix-awaitable-navigation
Closed

Update useAwaitableNavigate to handle navigations between pages reliably#2889
nikosdouvlis wants to merge 2 commits intorelease/v4from
nikos/fix-awaitable-navigation

Conversation

@nikosdouvlis
Copy link
Member

We need to use window to store the reference to the buffer, as ClerkProvider might be unmounted and remounted during navigations. If we use a ref, it will be reset when ClerkProvider is unmounted

Description

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:

@nikosdouvlis nikosdouvlis requested a review from a team as a code owner February 28, 2024 22:13
@nikosdouvlis nikosdouvlis requested review from anagstef and removed request for a team February 28, 2024 22:13
@changeset-bot
Copy link

changeset-bot bot commented Feb 28, 2024

🦋 Changeset detected

Latest commit: 86eb801

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

This PR includes changesets to release 4 packages
Name Type
@clerk/clerk-js Patch
@clerk/nextjs Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo 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 force-pushed the nikos/fix-awaitable-navigation branch from ccc301f to 91efa8f Compare February 28, 2024 22:13
@nikosdouvlis
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

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

Package Version
@clerk/chrome-extension 0.6.12-snapshot.v91efa8f
@clerk/clerk-js 4.70.2-snapshot.v91efa8f
@clerk/clerk-expo 0.20.7-snapshot.v91efa8f
@clerk/nextjs 4.29.9-snapshot.v91efa8f

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

npm i @clerk/chrome-extension@0.6.12-snapshot.v91efa8f --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@4.70.2-snapshot.v91efa8f --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@0.20.7-snapshot.v91efa8f --save-exact

@clerk/nextjs

npm i @clerk/nextjs@4.29.9-snapshot.v91efa8f --save-exact

@nikosdouvlis nikosdouvlis force-pushed the nikos/fix-awaitable-navigation branch from 91efa8f to c9a521b Compare February 28, 2024 22:30
@nikosdouvlis
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

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

Package Version
@clerk/chrome-extension 0.6.12-snapshot.vc9a521b
@clerk/clerk-js 4.70.2-snapshot.vc9a521b
@clerk/clerk-expo 0.20.7-snapshot.vc9a521b
@clerk/nextjs 4.29.9-snapshot.vc9a521b

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

npm i @clerk/chrome-extension@0.6.12-snapshot.vc9a521b --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@4.70.2-snapshot.vc9a521b --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@0.20.7-snapshot.vc9a521b --save-exact

@clerk/nextjs

npm i @clerk/nextjs@4.29.9-snapshot.vc9a521b --save-exact

@nikosdouvlis
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

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

Package Version
@clerk/chrome-extension 0.6.12-snapshot.vc9a521b
@clerk/clerk-js 4.70.2-snapshot.vc9a521b
@clerk/clerk-expo 0.20.7-snapshot.vc9a521b
@clerk/nextjs 4.29.9-snapshot.vc9a521b

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

npm i @clerk/chrome-extension@0.6.12-snapshot.vc9a521b --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@4.70.2-snapshot.vc9a521b --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@0.20.7-snapshot.vc9a521b --save-exact

@clerk/nextjs

npm i @clerk/nextjs@4.29.9-snapshot.vc9a521b --save-exact

@nikosdouvlis nikosdouvlis force-pushed the nikos/fix-awaitable-navigation branch from c9a521b to 41e6fdb Compare February 29, 2024 03:02
@nikosdouvlis
Copy link
Member Author

!snapshot

Copy link
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.

LGTM

@clerk-cookie
Copy link
Collaborator

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

Package Version
@clerk/chrome-extension 0.6.12-snapshot.v6eb1bf8
@clerk/clerk-js 4.70.2-snapshot.v6eb1bf8
@clerk/clerk-expo 0.20.7-snapshot.v6eb1bf8
@clerk/nextjs 4.29.9-snapshot.v6eb1bf8

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

npm i @clerk/chrome-extension@0.6.12-snapshot.v6eb1bf8 --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@4.70.2-snapshot.v6eb1bf8 --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@0.20.7-snapshot.v6eb1bf8 --save-exact

@clerk/nextjs

npm i @clerk/nextjs@4.29.9-snapshot.v6eb1bf8 --save-exact

@nikosdouvlis nikosdouvlis force-pushed the nikos/fix-awaitable-navigation branch from 6eb1bf8 to 1d4dfe6 Compare February 29, 2024 15:08
* https://nextjs.org/docs/app/building-your-application/caching#invalidation-1
*/
export const invalidateCacheAction = async () => {
cookies().delete(`__clerk_random_cookie_${Date.now()}`);
Copy link
Member

Choose a reason for hiding this comment

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

❓ According to the docs linked, calling router.refresh() will clear the cache. Why do we need to use the cookies approach?

Copy link
Member Author

@nikosdouvlis nikosdouvlis Feb 29, 2024

Choose a reason for hiding this comment

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

According to the docs, router.refresh() invalidates the cache for the current router not for all routes. If we are at '/path' and the '/' route is cached, navigating to / will use the cached payload even after firing refresh()

See: https://nextjs.org/docs/app/building-your-application/caching#invalidation-1

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I read it as it invalidates the router cache, not just for the current route, but if what you have here works or addresses some issue you experienced with router.refresh() then lgtm 😀

clerkNavRef.current = (to, opts) => {
return new Promise<void>(res => {
clerkNavPromiseBuffer.current.push(res);
startTransition(() => {
Copy link
Member

Choose a reason for hiding this comment

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

I believe removing the transition here will re-introduce the original issue, which is that state updates were happening during the router transition, or that the setActive call was not completing at all because of the navigation triggered by onBeforeSetActive.

Is there a way we can retain the use of the transition while keeping the buffer on the window?

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll most likely keep using the transition API - this is removed temporarily so I can cut a snapshot version for testing

Copy link
Member Author

@nikosdouvlis nikosdouvlis Feb 29, 2024

Choose a reason for hiding this comment

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

setActive call was not completing at all because of the navigation triggered by onBeforeSetActive

Regarding this one, I never managed to reproduce a failed setActive because of us not listening to isPending

I think the issue was caused by the components being unmounted during navigation but I do agree that keep using transition state is the way to go here

router.refresh();
router.push(window.location.href);
}
return invalidateCacheAction();
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't we want to invalidate the router cache in onAfterSetActive? Why in before?

Copy link
Member Author

Choose a reason for hiding this comment

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

We want to invalidate the router cache before a navigation occurs, otherwise the navigation might not fire atll . Currently, the order is: setActive > onBeforeSetActive > beforeEmit() > onAfterSetActive

@nikosdouvlis nikosdouvlis force-pushed the nikos/fix-awaitable-navigation branch from 1d4dfe6 to 40af74c Compare February 29, 2024 16:14
@nikosdouvlis
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

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

Package Version
@clerk/chrome-extension 0.6.12-snapshot.v40af74c
@clerk/clerk-js 4.70.2-snapshot.v40af74c
@clerk/clerk-expo 0.20.7-snapshot.v40af74c
@clerk/nextjs 4.29.9-snapshot.v40af74c

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

npm i @clerk/chrome-extension@0.6.12-snapshot.v40af74c --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@4.70.2-snapshot.v40af74c --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@0.20.7-snapshot.v40af74c --save-exact

@clerk/nextjs

npm i @clerk/nextjs@4.29.9-snapshot.v40af74c --save-exact

…tween pages reliably

We need to use window to store the reference to the buffer, as ClerkProvider might be unmounted and remounted during navigations. If we use a ref, it will be reset when ClerkProvider is unmounted
…tween pages reliably

We need to use window to store the reference to the buffer, as ClerkProvider might be unmounted and remounted during navigations. If we use a ref, it will be reset when ClerkProvider is unmounted
@nikosdouvlis
Copy link
Member Author

!snapshot

@clerk-cookie
Copy link
Collaborator

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

Package Version
@clerk/chrome-extension 0.6.12-snapshot.v86eb801
@clerk/clerk-js 4.70.2-snapshot.v86eb801
@clerk/clerk-expo 0.20.7-snapshot.v86eb801
@clerk/nextjs 4.29.9-snapshot.v86eb801

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

npm i @clerk/chrome-extension@0.6.12-snapshot.v86eb801 --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@4.70.2-snapshot.v86eb801 --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@0.20.7-snapshot.v86eb801 --save-exact

@clerk/nextjs

npm i @clerk/nextjs@4.29.9-snapshot.v86eb801 --save-exact

@nikosdouvlis
Copy link
Member Author

This released was included in the v5 beta release and so far everything seems to be working as expected.

However, we've had no other reports for unexpected behavior in v4. GIven that this fix could introduce breaking behavior for v4 apps, even if extremely unlikely, I'm closing this for now. We will merge and release if we receive any other reports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants