Closed toniopelo closed 3 years ago
I reversed the tuple values order in the useRouter()
return so it is compliant with the expected form of hook return ([value, setValue]
). Seemed a better choice.
Any thoughts on this @developit ?
Sorry to bother you again @developit, but would you mind take a look at this one ? This is working on my project since a few month already, seems all fine.
Up @developit 🌱 👁️
Update: Adding a parameter to useRouter() to allow typing the url parameters concerning the route or set of routes your component is in. If not given, this defaults to the original value
const [{ matches }] = useRouter<{id: string}>()
matches.id // matches is typed as { id: string }
cc @developit
@prateekbh Do you have any idea of somebody I could ping for this to be reviewed ?
Hiya! Sorry for the super long wait time on this - I'd read the PR and was noodling on what to do.
Two things on my mind:
originally I was thinking we should avoid hooks here and tell folks to use something like wouter, but given how straightforward this PR makes things, I'm okay with merging it.
I wonder if, because hooks is a separate library, this would make more sense to export from preact-router/match
?
For the second one, it might actually be a really nice way to get rid of the global subscriptions
/ Router.subscriptions
. match/src/index.js
could be simplified to:
import { h, Component } from 'preact';
import { useContext } from 'preact/hooks';
import { Link as StaticLink, exec } from 'preact-router';
/** imagine your useRouter implementation here */
// this was previously 24 lines of code! yay
export function Match(props) {
const { url, path } = useRouter()[0];
const matches = exec(path, props.path, {}) !== false;
return props.children({ url, path, matches });
}
// Link becomes simpler too! and is no longer global
export function Link({ class: c, className, activeClass, activeClassName, path, ...props }) {
const inactive = [c, className].filter(Boolean).join(' ');
const active = [c, className, activeClass, activeClassName].filter(Boolean).join(' ');
const path = useRouter()[0].path;
const matches = exec(path, props.path, {}) !== false;
return <StaticLink {...props} class={matches ? active : inactive} />;
}
export default Match;
Ah I also thought of one more thing while writing the above - it might be super useful to allow passing a path to useRouter()
:
function Foo() {
const [{ path, url, matches }] = useRoute('/profile/:username');
if (matches) {
return <Profile username={matches.username} />;
}
return <Homepage />;
}
(or maybe this would just be a second hook we could provide?)
Hi @developit!
Thanks for the review and I'm glad that this convinced you to merge it :)
I've applied your suggestions to simplify Match and Link components and got rid of the global Router.subscriptions
🎉
Concerning the useRouter hook being exported from preact-router/match
, I first was trying to imagine a good way to achieve that. As the RouterContext
, on which the hook value depends, is really tied to the core router, I figured out I would keep the context in the core package and export it from there, I would then import and use it within preact-router/match
.
My try is here
useRouter() always return the inital value of the hook, probably because it keeps a copy of the context when the module is first exported/imported and does not point to the same object reference
I might be missing something there, don't hesitate to share your thoughts.
Concerning the possibility to pass a path to useRouter('/my/path/:param')
.
I guess the goal is to reproduce the Match
use case. I think it's a good idea, but I would be more comfortable having matches
always being the params of the current route and having another props returned like isMatch
that would be a boolean that replicate the Match
behavior in case you pass a path to the hook.
Or we could have another useMatch
hook instead...
What do you think ?
Up @developit 😁
🎰 😄 @developit (FYI, I do use this in production for a while now, seems fine).
@developit 👋 😄
Would be great to have this merged @developit, if this is okay and because this has been a very long time this PR is opened, I think discussions about enhancing this feature can take place elsewhere after this is merged. Would you agree ?
@developit I don't know what to do apart from notifying you again, so I notify you again.
@prateekbh Do you see any way to have this merged ?
This is frustrating. cc @developit @prateekbh
Wow the hook has landed! Fantastic work here @toniopelo 🎉
Thanks a lot @marvinhagemeister and @prateekbh, feels good to have this merged finally 😛 💪
Hey guys, I see the PR is merged but there isn't any new release/tag version. So are we going to have a new release version to use this new useRouter
hook?
@marvinhagemeister Any plan on releasing this soon on npm ?
@marvinhagemeister @developit Up, could this be released ?
@prateekbh @developit @marvinhagemeister @masteroftheentireworldandtheuniverse Please, do something, I'm getting really tired with this situation. I would like to be able to npm i preact-router
and have the hook. It's been almost two years I'm using this in production with a specific build on my fork, this is really REALLY ReAlLy too long.
@toniopelo I think this is published already
https://www.runpkg.com/?preact-router@4.0.1/dist/preact-router.module.js
@JoviDeCroock Wooow that's great. I figured that, without any response to my previous comments, it wasn't yet published. But I was wrong, thanks a lot for the good news 🎉 😄 .
I'll be able to change all my package.json
and delete my fork, I'm a new guy 😛.
Fixes #339
This PR adds the
useRouter()
hook that leverage preactuseContext()
.Usage:
This PR also adds
path
andmatches
to theonChange
Router callback.