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

Add warnings for missing redirect destination(s) #320

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

L422Y
Copy link

@L422Y L422Y commented Dec 26, 2023

Add warnings for missing redirect destination(s)

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Add warnings for missing redirect destination(s) and disableWarnings setting to disable the warnings (for instance if a server endpoint is being used instead of a page)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why)

Add warnings for missing redirect destination(s) and `disableWarnings` setting to disable the warnings (for instance if a server endpoint is being used instead of a page)
Add `disableWarnings` documentation
Restore client options line to correct spot
Copy link
Contributor

nuxt-studio bot commented Dec 26, 2023

Live Preview ready!

Name Edit Preview Latest Commit
supabase Edit on Studio ↗︎ View Live Preview f2fe6ff

Copy link

netlify bot commented Dec 26, 2023

👷 Deploy request for n3-supabase pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit f2fe6ff

@L422Y L422Y changed the title Patch 1 Add warnings for missing redirect destination(s) Dec 26, 2023
@Atinux
Copy link
Collaborator

Atinux commented Dec 27, 2023

Thanks @L422Y for the PR

TwoSomenotes though:

  • What if the page is a dynamic route then? You won't be able to detect it even though the path could match
  • What about children routes?
  • For such feature, I think it is important also to get the server routes to have a good DX (see how the Nuxt DevTools does it)

I do think that this is too much work for a small advantage on the end user (because when testing the app, he can figure this out quite fast that the page does not exists)

@L422Y
Copy link
Author

L422Y commented Dec 27, 2023

A console warning seemed like a good DX compromise – I've spent about four hours on one project and then two on another trying to figure out where the redirect to "/login" was coming from, so from my experience it's a worthwhile 😅 especially if the values are sourced from the default options.

Tried to find the correct hook for the server endpoints but needed to move on and weighed the odds of someone using a server endpoint versus just using the defaults, and if they were using server endpoints they would be more likely to have configured the redirects / disabled warnings.

Intend(ed) on enhancing this with server endpoints as well.

We could pull some logic from DevTools or my sitemap generator for additional route checking, this pull request was mostly just to get it out of my head and into the real world :)

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.

None yet

3 participants