Skip to content

catch ERR_INVALID_URL#176

Merged
Fil merged 2 commits intomainfrom
fil/invalid-url
Nov 15, 2023
Merged

catch ERR_INVALID_URL#176
Fil merged 2 commits intomainfrom
fil/invalid-url

Conversation

@Fil
Copy link
Contributor

@Fil Fil commented Nov 15, 2023

closes #170

@Fil Fil requested a review from cinxmo November 15, 2023 11:21
let {pathname} = url;
try {
const url = new URL(req.url!, "http://localhost");
let {pathname} = url;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just clean the req.url so it doesn't end with 2 backslashes? or redirect? I think in this case it's ok to assume that it was just a typo

Copy link
Contributor Author

@Fil Fil Nov 15, 2023

Choose a reason for hiding this comment

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

Suppose you have a link in index.md to the "//" URL; now the dev server returns a 404 instead of crashing, which is what this PR wanted.

We could decide to redirect // to / on the dev server, but it would only hide this problem from the developer, as the link wouldn't work on the build. So my take is that it is better to be strict and error. (But more discussions on what to do on 404s are happening at #174 :) )

Copy link
Contributor

@cinxmo cinxmo left a comment

Choose a reason for hiding this comment

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

added a suggestion but there's nothing blocking this PR!

@Fil Fil enabled auto-merge (squash) November 15, 2023 15:29
@Fil Fil merged commit 047dce9 into main Nov 15, 2023
@Fil Fil deleted the fil/invalid-url branch November 15, 2023 15:30
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.

dev server crashes on http://127.0.0.1:3000//

2 participants