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

Issue #104: Routes without trailing slash lead to 404 #119

Merged
merged 7 commits into from
Dec 13, 2022
Merged

Conversation

macdonst
Copy link
Contributor

Signed-off-by: macdonst [email protected]

@tbeseda
Copy link
Contributor

tbeseda commented Dec 12, 2022

I agree with the text of issue #104 : it could redirect but I'm not sure we should render at that path.

This one is a bit more nuanced since trailing slashes have meaning here and while this app likely intends to render routing/index.md when a user hits /routing, it's technically different and there are some side effects.

  1. It would crash into /routing.md if we had one -- which we shouldn't add, but it's possible in the current .md router
  2. 404s get weird when the trailing slash is missing and index.md is added
  3. Also it messes up the "Edit on GitHub" link. For example, this /routing path creates this broken link.

IMO it should 302 redirect to /routing/ if it knows /routing/index.md exists.
So that would make it a part of the error handling instead of the render step. Basically use an existsSync check

@tbeseda
Copy link
Contributor

tbeseda commented Dec 12, 2022

OR we go with the alternative router I was kicking around where there are no index files.
So instead of routing/index.md + routing/lifecycle.md + the other nested .md files.
We'd have routing.md + routing/lifecycle.md + the rest still in the routing folder.

(btw, I am omitting the rest of the path (learn/concepts/.) above routing to make it easier to think about)

@macdonst
Copy link
Contributor Author

@tbeseda great feedback. What if we line up with the way Enhance does things and use:

routing.md + routing/lifecycle.md

@tbeseda
Copy link
Contributor

tbeseda commented Dec 12, 2022

@tbeseda great feedback. What if we line up with the way Enhance does things and use:

routing.md + routing/lifecycle.md

I like it! Symmetry is nice for my brain
fun fact: I wrote the "arcdown-router" package that became this site prior to Enhance routes being settled and I tried to base it on my PHP days, which in retrospect was the wrong model to follow 😜

@macdonst
Copy link
Contributor Author

@tbeseda unfortunately, if I rename the files, we have the opposite problem.

@macdonst
Copy link
Contributor Author

@tbeseda I ended up doing a redirect instead. Please give it a once over.

@macdonst macdonst merged commit d33c68e into main Dec 13, 2022
@macdonst macdonst deleted the issue104 branch December 13, 2022 16:54
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

2 participants