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 for React 16.7+ #6453

Closed
wants to merge 3 commits into from
Closed

Add useRouter hook for React 16.7+ #6453

wants to merge 3 commits into from

Conversation

mjackson
Copy link
Member

@mjackson mjackson commented Nov 3, 2018

Still experimental, but no reason we shouldn't be able to have a useRouter hook that works exactly like a path-less <Route>.

Please don't merge until React 16.7 is out of alpha! Actually, let's do what @Janpot suggested and release this in 4.x as unstable_useRouter.

See #6430

Still experimental, but no reason we shouldn't be able to have a
useRouter hook that works exactly like a path-less <Route>.

See #6430
@danielkcz
Copy link

danielkcz commented Nov 3, 2018

@mjackson Can't we have this released under some next dist-tag? Honestly, I don't see a point in having this PR here for another month or two without being able to use it. It's not a breaking change, it's purely additive and if someone uses it a wrong way, they get slapped.

@Janpot
Copy link

Janpot commented Nov 3, 2018

or name it unstable_useRouter until 16.7 is released. Analog to how react prereleases experimental features.

@mjackson
Copy link
Member Author

mjackson commented Nov 3, 2018

That's a great idea, @Janpot. Let's do that.

@remix-run remix-run deleted a comment from EloB Nov 6, 2018
@danielkcz
Copy link

@mjackson

That's a great idea, @Janpot. Let's do that.

Are you actually waiting for someone else to do that?

@mjackson
Copy link
Member Author

mjackson commented Nov 7, 2018

@FredyC No, I already did 8e2cd23

@danielkcz
Copy link

@mjackson Well, there are some other valid comments above, so I am just wondering what's the holdup. If you are busy, I am sure someone can finish this, but I think it's important to let us know.

@@ -0,0 +1,34 @@
import React from "react";
import ReactDOM from "react-dom";
import { MemoryRouter, Route, useRouter } from "react-router";

Choose a reason for hiding this comment

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

To fix tests failing :)

Suggested change
import { MemoryRouter, Route, useRouter } from "react-router";
import { MemoryRouter, Route, unstable_useRouter as useRouter } from "react-router";

Choose a reason for hiding this comment

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

@mjackson Is it really that hard to change this and release it? :) Looks like you are deliberately waiting for React 16.7 to come out and stalling :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Please be patient, @FredyC. Thanks :)

let context = useContext(RouterContext);
let location = options.location || context.location;
let match = options.path
? matchPath(location.pathname, options)
Copy link
Contributor

@edygar edygar Nov 20, 2018

Choose a reason for hiding this comment

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

You could useMemo here like:

let match = options.path
  ? useMemo(() => matchPath(location.pathname, options), [
      location.pathname,
      options.path,
      options.exact,
      options.strict
    ])
  : context.match;

So it won't loop through all routes at matchPath on every usage that route remains unchanged after a render.

Choose a reason for hiding this comment

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

I have a feeling that at the moment the useMemo came to the light, people are trying to memoize everything :) Bear in mind, that even such memoization has some overhead and without proper benchmarks, it's just a speculation what would be more performant.

Copy link

@danielkcz danielkcz Nov 20, 2018

Choose a reason for hiding this comment

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

Oh and also a conditional use of hook is a big NO-NO, so I really wouldn't do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ops! I'm really getting used to it.

And you're right, it looks like premature optimization for now.

@jadbox
Copy link

jadbox commented Dec 24, 2018

Any update on the status of the PR?

@danielkcz
Copy link

danielkcz commented Dec 28, 2018

@jadbox Sadly not, @mjackson is asking for patience for almost two months already and I am really not sure why is it such a problem to get this totally safe change released. Would love to at least hear some explanation if it's waiting on something or what :/

@Wezea
Copy link

Wezea commented Jan 25, 2019

Would be great to get this merged, as hooks gonna be released soon.

facebook/react#14679

@HaNdTriX
Copy link

HaNdTriX commented Feb 6, 2019

Hooks are stable now. https://github.com/facebook/react/releases/tag/v16.8.0

@danielkcz danielkcz mentioned this pull request Feb 6, 2019
@danielkcz
Copy link

danielkcz commented Feb 6, 2019

I think we should just make a new fresh PR. This one needs updating anyway and @mjackson seems busy discovering hooks world 😄

https://twitter.com/mjackson/status/1092503452301684736

@panjiesw
Copy link

panjiesw commented Feb 6, 2019

Seems like v5, built entirely with hooks, is already under consideration :)

@ianstormtaylor
Copy link

@mjackson could we get this one released?

This would be really useful to avoid render props for matching active state only. Thanks!

@NullVoxPopuli
Copy link

NullVoxPopuli commented Feb 18, 2019

how do we use this from git?
tried:

    "react-router": "github:reacttraining/react-router#8e2cd2380136e5781d012a2478635ab2ffb76e1b",

with yarn,
but:

error Can't add "react-router": invalid package version undefined.

which kiiiiinda makes sense, cause this is a monorepo.

@danielkcz
Copy link

@NullVoxPopuli I would kinda advise to copy & paste the code from this PR and use that instead. It's fairly simple functionality in a single file and later when it comes out officially, it should be easy to refactor.

I would only wish @mjackson would be more communicative regarding this 😕

@flq
Copy link

flq commented Feb 21, 2019

Before considering a full-on rewrite, I would just like to say that the useRouter - hook has great value in and of itself.

@@ -0,0 +1,20 @@
import React, { useContext } from "react";

Choose a reason for hiding this comment

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

Maybe you should remove the unused default React symbol import ?

@mdestagnol
Copy link

Any update on this @mjackson ? Would be great to use hooks for new projects instead of HOC. Thx a lot for this great library by the way!

@mjackson
Copy link
Member Author

We are planning to introduce some hooks soon, but I think useRouter may be too coarse. Instead, we're thinking:

  • useLocation() - gets you the current location object
  • useMatch(path) - gets you the match object for the given path
  • useParams() - gets you the current match.params

and possibly a few others. We are planning on pushing some of these soon in 5.x

@mjackson mjackson closed this Mar 19, 2019
@sorahn
Copy link

sorahn commented Mar 21, 2019

will useMatch, with no path, just give you closest match object? That would be super useful as you need match.path and match.url from the nearest match to build "relative" routes.

@lock lock bot locked as resolved and limited conversation to collaborators May 21, 2019
@timdorr timdorr deleted the use-router branch June 25, 2019 13:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.