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(router): remove all trailing slashes for path #26

Merged
merged 2 commits into from
Jul 7, 2024

Conversation

donkeyDau
Copy link
Contributor

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

@rschristian
Copy link
Member

What's the need for this?

@donkeyDau
Copy link
Contributor Author

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

@rschristian

This comment was marked as outdated.

@donkeyDau
Copy link
Contributor Author

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

This comment was marked as outdated.

@donkeyDau
Copy link
Contributor Author

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 ...

Copy link
Member

@rschristian rschristian left a comment

Choose a reason for hiding this comment

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

Upon more thought, you sold me.

If you want to add a test (I can do so a bit later, alternatively), let's just get this in.

src/router.js Show resolved Hide resolved
Copy link
Member

@rschristian rschristian left a comment

Choose a reason for hiding this comment

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

I appreciate you bearing with me.

I'm still back and forth on trailing slashes re:URLPattern, but this I'm fine with this for now. Bare is a bit nicer to look at anyhow, so I'm thinking bare will be the preference and we'll internally always coerce to it.

@rschristian rschristian merged commit 6cfff8b into preactjs:main Jul 7, 2024
1 check passed
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.

2 participants