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(nuxt): abort navigation when updating window.location #21521

Merged

Conversation

nicolaspayot
Copy link
Contributor

@nicolaspayot nicolaspayot commented Jun 11, 2023

πŸ”— Linked issue

#21314

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

On client side, when we use the navigateTo function to redirect to an external URL from a middleware (and when the Nuxt application is hydrated => while navigating from NuxtLink or navigateTo, ...), we abort the navigation so that the page component is not rendered before location changes.

Resolves #21314

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@nicolaspayot nicolaspayot force-pushed the fix/navigateTo-external-middleware-csr branch from 3fdcb7a to 485b88b Compare June 11, 2023 14:36
@nicolaspayot nicolaspayot force-pushed the fix/navigateTo-external-middleware-csr branch from ea3be0b to f668d32 Compare June 11, 2023 14:40
if (!nuxtApp.isHydrating) {
return false
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a corner case that's not handled here: when the Nuxt app is loaded on a route that has a redirect middleware. The page gets rendered (and is visible during a very short time) before the location changes. If we also return false when nuxtApp.isHydrating, it renders a 404 error / page (that's visible during a very short time).

There's something we could do: delay the return statement, like so, for example:

if (!nuxtApp.isHydrating) {
  return false
}
return new Promise(resolve => setTimeout(resolve, 10))

But it looks a bit hacky πŸ€”

Copy link
Member

Choose a reason for hiding this comment

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

I wonder what happens if you return a never-resolving promise πŸ€”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think that's what's happening with return new Promise(resolve => setTimeout(resolve, 10)) as the location changes before the resolve function gets called.

I like the idea of returning a never-resolving promise (return new Promise(() => {})) for this case πŸ‘

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielroe I've pushed the change. Let me know if this is acceptable. I can't see a case when this would lead to an issue. Maybe if, somehow, location.href = toPath or location.replace(toPath) fail but can this happen?

@nicolaspayot nicolaspayot marked this pull request as ready for review June 13, 2023 07:16
@danielroe danielroe changed the title fix(nuxt): abort navigation when redirecting to external link from middleware on client side fix(nuxt): abort navigation when updating window.location Jun 14, 2023
@danielroe danielroe merged commit 187230b into nuxt:main Jun 14, 2023
22 checks passed
@github-actions github-actions bot mentioned this pull request Jun 14, 2023
@danielroe
Copy link
Member

Thank you so much ❀️

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.

In CSR app, navigateTo (with external:true) redirection from route middleware triggers page script setup
2 participants