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

feat: redirect to previous path if its a 404 #1162

Merged
merged 4 commits into from
Nov 21, 2024
Merged

Conversation

crowlKats
Copy link
Member

@crowlKats crowlKats commented Nov 19, 2024

This is an MVP, not the perfect result, but much better than what we currently have (which is nothing)

@philhawksworth
Copy link
Member

I like it!

In future I think we can have some sort of combination of this logic, and a suitably populated 404 page with onward links to likely locations the user intended... but for now, this would be a valuable improvement

@josh-collinsworth
Copy link
Contributor

Question: what makes this a more desirable experience than ending up at a 404? Do we have lots of links where this behavior would be helpful (e.g., where there used to be a final part to a path that is no longer there)?

Because otherwise, I'm not sure ending up at a similar but still possibly wrong place is better. And unless we're pretty certain we are sending the user to the right place, I have misgivings at the prospect of a redirect that the user may not even realize took place.

@crowlKats
Copy link
Member Author

our current 404 page is terrible (just an empty page that says "Not Found"), and since our URLs are structured hierarchically, going to the next possible parent makes the most sense.
One example also is for some reason google indexed https://docs.deno.com/api/node/streams, which doesnt even exist, so going to the parent in this case is a better experience

@josh-collinsworth
Copy link
Contributor

josh-collinsworth commented Nov 19, 2024

I still think overriding the "correct" response is hazardous and unexpected behavior. Even if it works sometimes, it probably won't others, and that creates a confusing experience. (I also don't think it's probably a good thing that we make it impossible to even hit a 404, for a few reasons.)

I also notice that if the first redirect doesn't hit an existing page, it creates a "too many redirects" error.

Can we write some kind of "is this what you were looking for?" logic into the 404 page that pattern matches, or something along those lines? That seems much more in line with expected behavior. I just think this seems like a pretty haphazard solution, which could result in either better or worse user experience.

@josh-collinsworth
Copy link
Contributor

Happy to take a stab at this, if it helps

@BlackAsLight
Copy link

I think it may lead users to be confused about what just happened if there isn't something on the page telling them they were redirected due to a 404.

Like if I clicked a link and it just loaded the exact same page I'm on, I'd think this page is broken and not that the page I'm trying to go to doesn't exist. It would feel like a buggy experience.

@crowlKats
Copy link
Member Author

sure @josh-collinsworth, go ahead; i probably wont have have time to work on this anytime soon

@josh-collinsworth
Copy link
Contributor

josh-collinsworth commented Nov 21, 2024

I reworked this to be just a basic 404 page instead, which provides all the links in the runtime manual sidebar for the user to choose from.

Still not ideal, of course, but significantly more helpful than what we have, and a little less error-prone than trying to guess the correct path.

CleanShot 2024-11-21 at 10 05 42@2x

@@ -23,7 +23,7 @@ jobs:
- name: Set up Deno
uses: denoland/setup-deno@v2
with:
deno-version: canary
deno-version: 2.0.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Seems like an unrelated change

@josh-collinsworth josh-collinsworth merged commit afe9ad6 into main Nov 21, 2024
4 checks passed
@lucacasonato lucacasonato deleted the 404-redirects branch November 21, 2024 22:53
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.

5 participants