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

Add useRouter() hook and update Router.onChange #370

Merged
merged 12 commits into from
Sep 2, 2021

Conversation

toniopelo
Copy link
Contributor

@toniopelo toniopelo commented Jun 12, 2020

Fixes #339

This PR adds the useRouter() hook that leverage preact useContext().

Usage:

const [
    {
        router,
        url,
        previous,
        active,
        current,
        path,
        matches
    },
    route,
] = useRouter()

This PR also adds path and matches to the onChange Router callback.

@toniopelo toniopelo changed the title Add useRouter() hook update Router.onChange Add useRouter() hook and update Router.onChange Jul 3, 2020
@toniopelo
Copy link
Contributor Author

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 ?

@toniopelo
Copy link
Contributor Author

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.

@toniopelo
Copy link
Contributor Author

Up @developit 🌱 👁️

@toniopelo
Copy link
Contributor Author

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

@toniopelo
Copy link
Contributor Author

@prateekbh Do you have any idea of somebody I could ping for this to be reviewed ?

@developit
Copy link
Member

developit commented Dec 15, 2020

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:

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

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

@developit
Copy link
Member

developit commented Dec 15, 2020

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?)

@toniopelo
Copy link
Contributor Author

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 ?

@toniopelo
Copy link
Contributor Author

Up @developit 😁

@toniopelo
Copy link
Contributor Author

🎰 😄 @developit
(FYI, I do use this in production for a while now, seems fine).

@toniopelo
Copy link
Contributor Author

@developit 👋 😄

@toniopelo
Copy link
Contributor Author

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 ?

@toniopelo
Copy link
Contributor Author

@developit I don't know what to do apart from notifying you again, so I notify you again.

@toniopelo
Copy link
Contributor Author

@prateekbh Do you see any way to have this merged ?

@toniopelo
Copy link
Contributor Author

This is frustrating.
cc @developit @prateekbh

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

This is fantastic! 🙌

@marvinhagemeister marvinhagemeister merged commit 16f7d55 into preactjs:master Sep 2, 2021
@prateekbh
Copy link
Member

Wow the hook has landed! Fantastic work here @toniopelo 🎉

@toniopelo
Copy link
Contributor Author

Thanks a lot @marvinhagemeister and @prateekbh, feels good to have this merged finally 😛 💪

@felipeloge
Copy link

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?

@toniopelo
Copy link
Contributor Author

@marvinhagemeister Any plan on releasing this soon on npm ?

@toniopelo
Copy link
Contributor Author

@marvinhagemeister @developit Up, could this be released ?

@toniopelo
Copy link
Contributor Author

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

@JoviDeCroock
Copy link
Member

@toniopelo I think this is published already

https://www.runpkg.com/?preact-router@4.0.1/dist/preact-router.module.js

Screenshot 2022-01-28 at 15 36 04

@toniopelo
Copy link
Contributor Author

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

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.

Shouldn't this need hooks?
6 participants