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

Support more characters in path part matchers #129

Open
kyeotic opened this issue Nov 7, 2022 · 11 comments · May be fixed by #132
Open

Support more characters in path part matchers #129

kyeotic opened this issue Nov 7, 2022 · 11 comments · May be fixed by #132

Comments

@kyeotic
Copy link
Owner

kyeotic commented Nov 7, 2022

The current code for matching path segments to route props is quite simple: the regex: /:[a-zA-Z]+/. In at least one instance this surprised a user. As @coodoo points out This could be expanded to the following valid URL characters

; , / ? : @ & = + $ - _ . ! ~ * ' ( ) #

Since changing this is a breaking change for the library I am opening an issue to invite discussion on the topic.

The way I see it none of the following characters belong in a JS variable, so they should not be part of the route prop matcher.

; , / ? : @ & = +  - . ! ~ * ' ( ) #

That leaves: $ _. I think as part of a regex using $ could be confusing, and I also think its probably unlikely to be wanted. That leaves only _. I am willing to create a breaking change to add this, but only once there is critical mass for it. Breaking changes are a cost to the userbase, and I don't want to charge that cost if the benefit is to less than 5% of the users. Of course, I don't have a good way to poll, or even a good idea of how big the user base is, but I do have a good handle on the number of people active on this repository and its about 10. So if I can get 3 people who want a breaking change in order to have underscores in route props I will make the change.

I will leave the issue open for 1 year, or until that happens, whichever comes first. Of course, the floor is still open for anyone that wants to defend one of the characters I didn't like.

@coodoo
Copy link

coodoo commented Nov 8, 2022

Out of curiosity why would adding support _ a breaking change? Wouldn't it be a revise on the regex part will do the trick?

That being said, I'm totally fine with not supporting _, it's just a habit brought from those python days. 🤷🏻‍♂️

@n33pm
Copy link
Contributor

n33pm commented Nov 8, 2022

const routes = {
  "/users/:userId": ({ userId }) => <div>{userId}</div>,
  "/users/:userId_test": ({ userId }) => <div>{userId}</div>
};

currently the second route would match with /users/n33pm_test and userId = n33pm.
after the change userId_test = n33pm_test and userId is undefined and both routes would match the same.
probably it breaks ?!

@n33pm
Copy link
Contributor

n33pm commented Nov 8, 2022

and back to the question. If we change it I would add a start/end character like /user/:{userId} because something like /users/something:userIdsomething doesn't work and we could deprecate with /:[a-zA-Z]+/g and add an extra /:{[a-zA-Z0-9$_]+}/gU or keep both.

@kyeotic
Copy link
Owner Author

kyeotic commented Nov 10, 2022

Sorry @coodoo I thought I responded to this, but I must not have hit enter.

It's a breaking change because routes that match with an underscore will get a different variable passed in as a route parameter. That is the front-door API for the entire library, and changing the output like that would cause a previously functioning route to start failing. I know it seems like a minor addition to a regex, but that regex is the API for useRoutes.

@kyeotic
Copy link
Owner Author

kyeotic commented Nov 10, 2022

If we change it I would add a start/end character like /user/:{userId} because something like /users/something:userIdsomething doesn't work and we could deprecate with /:[a-zA-Z]+/g and add an extra /:{[a-zA-Z0-9$_]+}/gU or keep both.

That is an interesting case, but its rather easy to strip off the beginning of that value inside the route handler. I don't think its worth the complexity to support what is probably a very edge case.

'/users/:userId': ({ userId}) => (<Comp userId={userId && userId.replace('something', '')} />)

@rkok
Copy link

rkok commented Aug 14, 2023

@kyeotic Is there any workaround for this one that allows us to still have dots in paths?

@kyeotic
Copy link
Owner Author

kyeotic commented Aug 14, 2023

@kyeotic Is there any workaround for this one that allows us to still have dots in paths?

Yes, dot is part of the set of characters we can expand to support. But so far there has not been any interest in taking the breaking change in order to add that support.

@rkok
Copy link

rkok commented Aug 18, 2023

@kyeotic Understood. Well, I'm all for expanding it as one of the projects I'm working on requires dots in a path part matcher at the moment. :-)

How do you feel about having an option for useRoutes called "expanded path part matching" or so, defaulting to false to not break existing applications?

@kyeotic
Copy link
Owner Author

kyeotic commented Aug 18, 2023

I think that's a pretty good idea. I'm in the middle of a move currently, so I wont have time for any development for a bit. I would absolutely take a PR for this feature though 😄

@rkok
Copy link

rkok commented Aug 18, 2023

@kyeotic Upon closer inspection, it turned out that it wasn't Raviger, but Vite that was causing my problem with dots: vitejs/vite#2245 😵

Nevertheless I wrote a PR anyway - it's coming your way in a minute.

@rkok rkok linked a pull request Aug 18, 2023 that will close this issue
@kyeotic
Copy link
Owner Author

kyeotic commented Aug 18, 2023

Re-reading the intro again, I no longer agree with my own statement

The way I see it none of the following characters belong in a JS variable, so they should not be part of the route prop matcher.

The important factor isn't whether these characters are valid javascript variable names. That's an implementation detail of the language, and the URL is part of the application's interface. The important factor is whether those characters belong in a URL, and to that end raviger should aim to support everything that the browser supports.

Since this would be a breaking change I plan on taking @rkok's route and adding a route matching parameter to expand character matching support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants