search

preactjs / preact-iso

Isomorphic utilities for Preact
MIT License
69 stars 9 forks source link

feat(router): remove all trailing slashes for path #26

Closed donkeyDau closed 3 months ago

donkeyDau commented 4 months ago

Remove trailing slashes but ensure that a single '/' is present.

rschristian commented 4 months ago

What's the need for this?

donkeyDau commented 4 months ago

For stripping trailing slashes on the path. I realized that a single trailing slash is stripped but multiple are not.

rschristian commented 4 months ago

For stripping trailing slashes on the path

Well yes, I understand that. What issue does it address? Unless there's an issue in the lib impl, I'd personally call this user error.

donkeyDau commented 4 months ago

I was testing around and noticed that stuff breaks if I want to get the last path part on splitting on / and deliberately adding slashes at the end of the path. I've seen that there's a trailing slash handling but not dealing with all the slashes.

Your answer indicates that the trimming of the one trailing slash is a need of the library. If that's the case then imho it should not be exposed as the user (or at least I'm) either expects getting the unchanged path or fully trimmed by trailing slashes.

rschristian commented 4 months ago

stuff breaks if I want to get the last path part on splitting on /

This isn't a valid way to extract path segments unfortunately. // per the spec is entirely valid, it's an empty path segment. Using .split erases this.

Regex is your best bet, group between forward slashes.

Your answer indicates that the trimming of the one trailing slash is a need of the library.

My answer is that this is a breaking change (albeit over a rarely used URL feature) without much reasoning for it. I tend to think we should avoid changing things (in any repo) without strong reasoning, be it a bug or a feature.

I don't know why this was originally added (you might be able to find the context here or in WMR) but I'm not sure past reasons count for much?

donkeyDau commented 4 months ago

I can agree with that. To me this looks flawed that's why I wanted to make it "right". I'm also fine removing the whole line. Anyhow this would be a breaking change. Maybe it'll be integrated in a v3 ...