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

Allow <Link to> to accept an object #2177

Closed
mjackson opened this issue Oct 6, 2015 · 26 comments
Closed

Allow <Link to> to accept an object #2177

mjackson opened this issue Oct 6, 2015 · 26 comments
Labels

Comments

@mjackson
Copy link
Member

mjackson commented Oct 6, 2015

@gaearon suggests here that we could allow users to specify an object in their <Link to>. Then they could do something like:

<Link to={{ pathname: '/the/path', query: { the: 'query' }, hash: '#the-hash' }} />

This would deprecate the <Link query>, <Link state>, and (potential) <Link hash> props and provide one place for people to put all their location-related data. It also makes it easy for people to see how they can wrap up "named routes" into a simple function:

function chapter(number, verse) {
  return {
    pathname: `/chapters/${number}`,
    hash: `#verse-${verse}`
  }
}

<Link to={chapter(1, 3)}>Chapter 1, verse 3</Link>

I personally like the idea. Just wanted to start some discussion here to see what others think.

@timdorr
Copy link
Member

timdorr commented Oct 6, 2015

👍 in my humble opinion.

As a matter of convenience, what about making location objects interchangeable with strings universally? That's tangentially related, so I can create another issue for discussion on the topic if you'd rather separate it out.

@mjackson
Copy link
Member Author

mjackson commented Oct 7, 2015

what about making location objects interchangeable with strings universally?

That's an interesting idea. Feel free to open another issue where we can discuss further :)

@taion
Copy link
Contributor

taion commented Oct 8, 2015

This looks really cool - it cleans up the API quite a bit.

@dozoisch
Copy link
Contributor

Just curious. What would happen when are using hash history and specify pathname and hash?

Right now just doing a <Link to="mypath"> will be handled correctly by all history types.

@taion
Copy link
Contributor

taion commented Oct 15, 2015

Should we close this in favor of #2186? I think that would supersede this issue.

@timdorr
Copy link
Member

timdorr commented Oct 21, 2015

Yeah, we've got two issues on the same topic. Everyone move over to #2186!

@timdorr timdorr closed this as completed Oct 21, 2015
@adamziel
Copy link

adamziel commented Nov 5, 2015

Since people seem to need it, and #1840 is closed and no further discussion is possible in there, and this is the first linked issue, and the discussion is spread over 10 or so different issues, I am posting this in here as well:

https://github.com/adamziel/react-router-named-routes
Drop-in support for named routes, almost no refactoring required in your software

Sorry for the off-topic guys, it would be best if I was able to post it in the original issue;

@timdorr
Copy link
Member

timdorr commented Nov 5, 2015

@adamziel you're free to create a new issue to discuss the possibility of reversible routes. A PR is best, since you can discuss a concrete change vs a theoretical.

@adamziel
Copy link

adamziel commented Nov 5, 2015

@timdorr as far as I understand a PR into the core would not be possible since named routes will only work as long as one does not use the async routes (which was mentioned by a guy in the other issue). A separate package on the other hand may provide it while deliberately breaking the fancy functionality.

@gaearon
Copy link
Contributor

gaearon commented Nov 5, 2015

Indeed, we intended this to be supported in userland as a separate library (at least at first).
I filed adamziel/react-router-named-routes#1 as we intended this to be implemented slightly differently, without monkeypatching the router.

@adamziel
Copy link

adamziel commented Nov 6, 2015

I adjusted the code accordingly to your suggestion; If I issue a PR to include a link to that repo in UPGRADE_GUIDE.md would you merge it?

@taion
Copy link
Contributor

taion commented Nov 6, 2015

I'm a bit ambivalent here. I like the idea of linking to a userspace named routes solution, but I'm a bit uncomfortable with giving the current solution an official imprimatur because it's somewhat incomplete.

Thinking about this a little bit more, I think a correct solution would be something building off #2186 that presents as a history enhancer with the ability to both automatically set up names from sync route definitions and accepts some sort of mapping configuration for async routes.

With #2186, this would then work for e.g. both <Link to> and history.push, and have full support for the 1.0 RR feature set.

What do you think?

@adamziel
Copy link

adamziel commented Nov 6, 2015

Sounds like a good idea, I'll take a look at this and maybe come up with something.

@taion
Copy link
Contributor

taion commented Nov 6, 2015

Cool, thank you! #2186 is probably a little ways away because our main focus right now is shipping 1.0.0-final, but it offers a lot of cleanup possibilities, including this.

@mwhite
Copy link

mwhite commented Nov 18, 2015

I would have posted this sooner on #1840, but didn't expect version 1.0 to actually get released without named routes.

React Router is an amazing library, and thanks for all your work on it, but removing such a fundamental feature with such weak justification throws its future for serious use into serious doubt.

I'd imagine I'm far from the only one who doesn't intend to upgrade to any version of this library that doesn't include named routes without having to install an external library, and I can't imagine any valid reason it should have to involve a wrapper function like <Link to={wrapper(name)} />.

I apologize to whatever extent these things have already been said:

Dynamically loading route config (i.e. getChildRoutes) means that we may not actually be able to build real URLs for s whose route config has not yet loaded

Is it possible to robustly support dynamic route loading and named routes at the same time?

A few possibilities immediately come to mind:

  1. <Router allowNamedRoutes>

    If true, throws an error if you define getChildRoutes(). If false, throws an error if you use .

    Can you really not maintain an API like this for now for people who want to use named routes and don't immediately require dynamic loading?

  2. <Link to={'name'} /> throws an error if a Route named 'name' is not known about yet.

    Users must explicitly pre-load any route that they wish to link to from a component if it's not a child, ancestor, or ancestor sibling route of the current route.

  3. <Link to={'parentRouteName.childRouteName'}/>

    Namespaced named routes. Necessary routes are loaded dynamically when making links.

  4. Just load the entire route hierarchy up front, or as soon as an unknown route name is referenced.

    Thanks to getComponents(), this does not have to also load components unless users want it to.

    While in addition to the performance benefits of code splitting, getChildRoutes() also improves maintenance of large applications by not having all routes in the same file, etc., shouldn't the default assumption with dynamic routes -- or should we say, modular/pluggable/etc -- in fact be that all routes are bundled into the same file for deployment, thus eliminating any downsides to loading all routes initially (or even on every route transition, since state passed to getChildRoutes() via <Link> may change), thus solving the named routes issue?

How about an API something like this:

let myRoute = {
  getDynamicChildRoutes(location, cb) {
    ...
  },
  getChildRoutes(cb) {
    ...
  }
};

Only one of getDynamicChildRoutes() and getChildRoutes() can be defined.

The router recalculates all descendant routes of all routes that define getDynamicChildRoutes() before each router transition so <Link name>s are correct, OR...

It results in undefined behavior to return routes with the same name from multiple places in the route tree structure, even if there is no location which would result in a single route tree containing duplicate route names. If a <Link> references an unknown route name, the router synchronously traverses the dynamic parts of the route tree until it finds a route with that name (hmm, that wouldn't work unless you make getChildRoutes() only synchronous).

We want to encourage users to NOT change their URLs–use a instead

Not only do URLs change before they're released, but not all web development occurs in a real-world niche where it's ideal to maintain redirects for every old URL.


getChildRoutes() is great for code-splitting, but it seems like there's a lot of complexity being added to support the completely separate use-case of dynamic child routes with location, which seems questionable (can someone point me to an example of something that can't be implemented with only component={SingleComponent} childRoutes={[() => require('route')]}, or an argument as to why the alternative is better?), and at the very least doesn't seem like it's worth removing what everyone thought was a fundamental feature of a router, named routes.

@timdorr
Copy link
Member

timdorr commented Nov 18, 2015

<Router allowNamedRoutes>

That means we have a boolean mode switch at the top of our configuration, which basically results in two different libraries. The kind of things that need to be changed to support named routes are too systemic to simply flip on or off.

<Link to={'parentRouteName.childRouteName'}/> Necessary routes are loaded dynamically when making links.

Loaded from where? This isn't a terrible idea or anything, but there's no clear mechanism for instructing router where to load those additional routes from.

Just load the entire route hierarchy up front

Doesn't work for large, code split apps or asynchronously-loaded routes. We are already running into this kind of thing as JSPM is gaining popularity. We can't assume we know the entirety of the route tree initially and have to run things async. This means we can't absolutely know the value of a named route ahead of time.

As it's been pointed out before, the real solution to what everyone is looking for is a reversible routing solution. Building a bidirectional router is hard. Really hard. We may get there eventually, but for now it's best if we work on solutions externally and eventually merge in something that proves to hit all the right notes and be high quality. We're already doing this with scroll behavior and async properties. Reversible routes even have some initial solutions. I think we can come up with a few different options that solve different specific needs.

@mwhite
Copy link

mwhite commented Nov 19, 2015

I'm not a routing expert, I have no idea what bi-directional routing is, and I may not have a full understanding of a lot of the fundamental building blocks and current developments. I'm just trying to explain how I think you can implement the functionality that existed previously (named routes) and code splitting (which I also value extremely highly) at the same time for most use cases.

I'm also saying that code splitting of routes in development does not imply code splitting of routes in production, and there's a good case to be made that the default use case decisions should be based on is bundling all routes in one file, which avoids the async performance issue for most cases as far as I can tell.

There seem to be several different desired capabilities:

  1. code splitting
  2. asynchronous route loading as a consequence of the fact that any script download initiated from JS is asynchronous
  3. asynchronous route loading for other purposes, i.e. making a request to an API to determine which routes to return
  4. location-dependent child routes
  5. named routes

I guess what I was arguing is that 5 is not an issue if all routes are known on app load, routes can be known on app load without excessive wait times if they are bundled into the same file, (I believe) bundlers exist today that are capable of handling this organization, 2 is not an issue if all routes are bundled in the same file, and 3 and 4 are not important enough to remove 5 because supporting 5 would require extra complexity to support 3 and 4.

Basically I'm just thinking of this as the minimal API for code splitting, seeing that it's easy to get all the routes for an app upfront (whether synchronously due to bundling in production, or asynchronously in development using a module loader that somehow supports asynchronous non-top-level requires - does that even exist? is it possible?), and wondering if there isn't some way to have named routes and code splitting while perhaps making the handling of dynamic routes and other asynchronous routes slightly more complex and/or require additional configuration.

let route = {
  component: () => require('component'),
  childRoutes: [
    () => require('route1'),
    () => require('route2')
  ]
}

@mwhite
Copy link

mwhite commented Nov 19, 2015

In short: if you synchronously load child routes (or effectively synchronously), asynchronously load components, and bundle all routes into the same file for production, that allows code splitting and (easily implementable but also correct) named routes, is probably optimal for the vast majority of applications, and doesn't prevent doing any of the other things that have been done, but just might make them slightly more complicated.

@mwhite
Copy link

mwhite commented Nov 19, 2015

Necessary routes are loaded dynamically when making links.

Loaded from where? This isn't a terrible idea or anything, but there's no clear mechanism for instructing router where to load those additional routes from.

Sorry, if 'parentRouteName' is replaced with 'topLevelRouteName' it's probably clearer. Each link name would be an explicit full path to the desired route based on the names of its ancestor routes (all routes would require a name). If a path is partially unknown, each level route would be loaded in turn, then it's child routes, until the leaf route is loaded.

I probably wouldn't advocate this over the solution above, but it would be easier.

That probably also runs into issues with render() being synchronous. There could be workarounds for that though.

@taion
Copy link
Contributor

taion commented Nov 19, 2015

This issue is really not the place to discuss named routes.

We understand that many of our users want to have named routes, but at this point in time given the viability of implementing named route support outside of React Router core, we're looking much more at user-space solutions (and time permitting may release a preferred one of our own).

@mjackson's summary in the OP of #1840 broadly covers the reasons why we don't think using route names is a preferred approach for front-end routing. At the same time, we're interested in providing support for use cases our users care about - there will be resolution here, but at this time it's most likely not going to take the form of <Route name> being a first-class concept in React Router, simply because it doesn't need to be.

@xogeny
Copy link

xogeny commented Nov 24, 2015

I don't really care so much about specifically having a name prop on a Route, I just wanted to avoid specifying the path information in more than one place. Everybody kept saying "you can do this in user space", but it wasn't clear to me how. Ultimately, I found a relatively simple solution that works "out of the box" in react-router 1.0 and I didn't see it mentioned here so I just wanted to comment on it for those who come poking through here looking (like I did) for a way to keep things DRY.

The first thing you need is formatPattern which can be imported with:

import { formatPattern } from 'react-router/lib/PatternUtils';

With this, you can push parameter values back into the path with a call like this:

  <Link to={formatPattern('/foo/:id', {id: 25})}>Foo 25</Link>

So on thing you can do is define all your paths somewhere centrally as strings and then reference them, e.g.,

  <Link to={formatPattern(fooPath, {id: 25})}>Foo 25</Link>

Or you can build create a PlainRoute object and use it both to define your Route and use it with Links, e.g.,

var fooPath = {
    path: "/fmu/:id",
    component: ...
};

<Route {...fooPath}/>
<Link to={formatPattern(fooPath.path, {id: 25})}>Foo 25</Link>

Bottom line is you don't actually need any special components, extra NPM modules, etc. to address the DRYness issue. That was sufficient for me. HTH

@remix-run remix-run locked and limited conversation to collaborators Nov 24, 2015
@remix-run remix-run unlocked this conversation Nov 24, 2015
@taion
Copy link
Contributor

taion commented Nov 24, 2015

@xogeny

I think that's a pretty good implementation of the pattern @mjackson proposes in the OP given the current API. That said, though, I think we can do better - one limitation is that what you have doesn't work particularly well for nested routes with nested path segments.

Eventually I'd like to move toward pluggable transitions, such that you could do with a history extension e.g.

import useNamedRoutes from 'use-named-routes'

const routes = <Route name="foo" path="/foos/:fooId" />
const history = useNamedRoutes(createBrowserHistory)({routes})
const router = <Router history={history} />

/* ... */

const link = <Link to={{name: 'foo', params: {fooId: 25}}}>Foo</Link>

We're not there yet, though, and there's a lot of groundwork that needs to go into place before we can get there.

@mwhite
Copy link

mwhite commented Nov 24, 2015

Where is the appropriate place to discuss named routes, since the original issue was closed?

I hope you don't mind if I summarize what I already said:

  • There is no way to implement named routes correctly, in user-space or otherwise, if any of your routes have "location-dependent child routes", unless you either assume that none of the location-dependent child routes have names, or reload the entire location-dependent parts of the route hierarchy before every transition to ensure that links are correct.
    • Currently, "dynamic routes" for the purpose of code splitting and "location-dependent child routes" are defined using the same API (getChildRoutes()), so there is no way for an implementation of named routes to know whether it's being used in a completely invalid way.
  • Named routes can easily be implemented if the entire route hierarchy is loaded upfront, which does not prevent code splitting -- simply bundle all routes in the same file and define getComponent(). If your app is so big that you don't want to load all routes at load time, then bundle your routes into separate bundles, and make any component explicitly depend on any routes it needs to link to if it can't be sure they will already have been loaded by the time it's rendered.

@taion
Copy link
Contributor

taion commented Nov 24, 2015

@mwhite

That's mostly correct, the same idea as on #2244 will allow explicitly mapping names to paths, if that's your cuppa.

That said, the difficulty is really setting up the API such that we can nicely build named routes in a principled way as an extension at the lowest level possible. I think what I have above outlines a way forward.

@timdorr
Copy link
Member

timdorr commented Nov 24, 2015

Hey guys, I locked this because it's really off base from the original issue. It's at best tangential, which results in us hiding valuable discussion in an issue that isn't apparent as to its contents.

I would highly recommend opening a new issue with a specific proposal for change. Rather than just beating the "we want named routes" drum, a possible solution gives us something actionable to discuss. I would look to @jlongster's excellent issue (technically a PR) on Redux: reduxjs/redux#569 While it wasn't merged in, the discussion was fantastic and spawned some great external libraries for side effects in Redux. I would hope we can achieve the same thing here.

Anyways, I won't re-lock, but hopefully we can move discussion to a more appropriate issue or PR.

@taion
Copy link
Contributor

taion commented Nov 24, 2015

@timdorr

Sorry about that. The whole named routes thing is delicate enough that I don't want to give it more visibility. The stuff I've been thinking about in the context of routing in the RN context will have named routes fall out as a natural consequence (but as a history enhancer rather than part of RR core). I think @xogeny's post was relevant because it's essentially just an implementation of the pattern that is proposed in the OP, although we are getting a little sidetracked at this point.

The basic summary is that

  • Something is coming
  • There is no ETA
  • You should probably drop named routes anyway

@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

8 participants