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

v2 Roadmap #2646

Closed
4 tasks done
ryanflorence opened this issue Dec 5, 2015 · 97 comments
Closed
4 tasks done

v2 Roadmap #2646

ryanflorence opened this issue Dec 5, 2015 · 97 comments
Milestone

Comments

@ryanflorence
Copy link
Member

This might not be the best way to manage this (one gigantic issue), but @mjackson and I are a solid 50% done with all of this anyway, so I'm not too concerned.

@taion you'll likely have some questions since this affects a lot of what you're trying to do, please chat me up on discord before we clog up this issue with back-and-forth conversation :)

Router 2.0 Roadmap

TOC

Motivations for the following API changes are all about cleaning up and clarifying the coupling between React Router and History; this generally translates to "You won't need to know anything about History anymore."

  • Learning the router is complicated by History, you have to learn both APIs and the line between them is unclear. This affects end users trying to use the libraries together as well as contributors (for example duplication of docs is also a problem).
  • We have some rough edges around ingration with libraries like React native, Redux, Relay, and our own AsyncProps.
  • React 15 is only going to allow components to export only one object to "context" (we only want one anyway)

The following issues should help address these concerns.

Note about upgrading

All v1.0.0 APIs will continue to work in v2.0.0. An app using only public API should be able to drop in v2.0.0 and just get warnings. Old API will be removed in 3.0.0. If you update your code and no longer get warnings, you should be able to upgrade to 3.0 w/o trouble.

Ship History 2.0

It's really just removing deprecated API. The rest of these items all kind of depend on it though.

Refactor route matching out of useRoutes and into match

Background

Currently we wrap a history with useRoutes so it is now aware of routes and performs the match against them. This changes the signature and timing of listen (making it async) causing difficulty when integrating or using alongside libraries like redux and react native.

Additionally, navigating outside of components (like in flux and redux) is complicated by Router wrapping the history provided as a prop.

Goals

  • Remove need for listenRoutes to integrate with other libs
  • Make navigating outside of components simpler for redux/flux etc.
  • Not require apps to wrap their singleton history in things like
    useQueries in order to navigate outside of components
  • apps can still learn about History and add behavior if they choose,
    (custom queryString parsing, etc.) and Router should not touch the
    history provided

Upgrading

////////////////////////////////////////////////////////////////////////////////
// Rendering
// v1.0.0
import createBrowserHistory from 'history/lib/createBrowserHistory'
render(<Router history={createBrowserHistory()}/>, el)

// v2.0.0
import { browserHistory } from 'react-router'
render(<Router history={browserHistory}/>, el)

////////////////////////////////////////////////////////////////////////////////
// Navigating outside of components

// v1.0.0
// probably exported from a module somehwere like this
import createBrowserHistory from 'history/lib/createBrowserHistory'
import useQueries from 'history/lib/useQueries'
const createHistory = useQueries(createBrowserHistory)
export default createHistory()
// and then used in an action or somehwere
import appHistory from './appHistory'
appHistory.push(...)

// v2.0.0
// React Router exports a singleton
import { browserHistory } from 'react-router'
browserHistory.push(...)

Tasks

While the nitty gritty won't be this simple, this is what will probably need to happen:

  • remove useRoutes and move all route matching to match
  • use match inside of Router instead of wrapping the history provided in anything (don't alter history)
  • create hashHistory and browserHistory singletons, exported from index.js
  • remove default to hashHistory and require devs to provide one, should help with people not understanding that there's a better option, and allow devs to decrease the bundle size of their app

Export only router to context

Background

Right now we put history, location, and route (sometimes) on context, we also pass a bag of props to route components. Turns out there's some overlap between them. We're tempted to add more to context, and add more to props because the line and purpose between them isn't clear.

React Router continues to strive to be just one library in a wonderful ecosystem of other libraries, prescribing strategies for data flow in an app is not our goal. We will put only what we absolutely need in context (which can be summed up with "what does Link need?). Everything else will go to route components as props. The opinions of the app can take over from there.

Context was recently documented, so we no longer need the mixins to hide the context API. It's up to the app to decide if it wants to use context directly or continue to hide it with mixins or higher order components.

Also, react 15 or 16 will only allow one contextType. We've been talking about only have one context type for a while. Now is the time.

Finally, in the global effort to make History an implementation detail, we need to provide something else to navigate around in components.

Goals

  • Make separation between what is on context v. route props clear, and remove overlap
  • Let apps decide how to get props the router gives them down the render tree
  • Remove need to learn History API

Tasks

  • Deprecate all items on context: history, location, route

  • Add context.router that looks like this:

    router: shape({
      push: func,
      replace: func,
      isActive: func,
      addRouteLeaveHook: func
    })
  • Pass router as a prop to route components

  • Deprecate all mixins

  • Deprecate passing history to route components

  • proxy push/replace/isActive/addRouteLeaveHook to underlying history (so integration with a custom history will work)

  • automatically remove route leave hooks since we should know everything we need to know, so devs don't have to and there's no reason to leave them hanging around.

Upgrading

History methods

In all cases where you once had a history for navigation, you now have a router with different methods.

// v1.0.0
history.pushState(state, path, query)
history.replaceState(state, path, query)

// v2.0.0
router.push(path)
router.push({ path, query, state }) // new "location descriptor"

router.replace(path)
router.replace({ path, query, state }) // new "location descriptor"

Navigating in route components

// v1.0.0
class RouteComponent extends React.Component {
  someHandler() {
    this.props.history.pushState(...)
  }
}

// v2.0.0
class RouteComponent extends React.Component {
  someHandler() {
    this.props.router.push(...)
  }
}

Navigating inside deeply nested components

// v1.0.0
const DeepComponent = React.createClass({
  mixins: [ History ],

  someHandler() {
    this.history.pushState(...)
  }
}

// v2.0.0
// You have a couple options:
// Use context directly (recommended)
const DeepComponent = React.createClass({
  contextTypes: {
    router: object.isRequired
  },

  someHandler() {
    this.context.router.push(...)
  }
}

// create your own mixin:
const RouterMixin = {
  contextTypes: {
    router: object.isRequired
  },
  componentWillMount() {
    this.router = this.context.router
  }
}

const DeepComponent = React.createClass({
  mixins: [ RouterMixin ],
  someHandler() {
    this.history.pushState(...)
  }
}

// use the singleton history you are using when the router was rendered,
import { browserHistory } from 'react-router'

const DeepComponent = React.createClass({
  someHandler() {
    browserHistory.push(...)
  }
}

Lifecycle Mixin with route components

// v1.0.0
const RouteComponent = React.createClass({
  mixins: [ Lifecycle ],
  routerWillLeave() {
    // ...
  }
})

// v2.0.0
const RouteComponent = React.createClass({
  componentDidMount() {
    const { router, route } = this.props
    router.addRouteLeaveHook(route, this.routerWillLeave)
  }
})

// or make your own mixin, check it out in the next section

Lifecycle Mixin with deep, non-route components

// v1.0.0
const DeepComponent = React.createClass({
  mixins: [ Lifecycle ],
  routerWillLeave() {
    // do stuff
  }
})

// v2.0.0
// you have a couple of options
// first you can put the route on context in the route component
const RouteComponent = React.createClass({
  childContextTypes: {
    route: object
  },

  getChildContext() {
    return { route: this.props.route }
  }
})

// and then access it on your deep component
const DeepComponent = React.createClass({
  contextTypes: {
    route: object.isRequired,
    router: objec.isRequired
  },

  componentDidMount() {
    const { router, route } = this.context
    router.addRouteLeaveHook(route, this.routerWillLeave)
  }
})

// or make your own mixin that will work for both route components and
// deep components alike (as long as your route component puts `route`
// on context
const Lifecycle = {
  contextTypes: {
    route: object.isRequired,
    router: objec.isRequired
  },

  componentDidMount() {
    const router = this.context.router
    const route = this.props.route || this.context.route
    router.addRouteLeaveHook(route, this.routerWillLeave)
  }
}

Add render prop to Router, remove RoutingContext prop

Background

The functional composition of history is awesome, but we also need this kind of composition for React Router when it's time to render. While we can add some behavior into history we lack the necessary lifecycle hooks that components give us. Integrations also need all of the routing information in aggregate (like all the components and routes).

Our current workaround is the RoutingContext prop, but that only lets us add one level of behavior. Consider an app that was built with Async Props, but now wants to iterate to using Relay, as well as add a new scroll restoring behavior. If each of these components used the RoutingContext prop API, they would all render a RoutingContext and therefore can't be used together.

Goals

  • Allow integration for middleware components that need lifecycle hooks and the aggregate routing information
  • Allow multiple levels of behavior, not just one

Upgrading

// v1.0.0
render(<Router RoutingContext={AsyncProps}/>, el)

// v2.0.0
render((
  <Router render={(props) => (
    <AsyncProps {...props} render={(props) => (
      <RoutingContext {...props} />
    )}/>
  )}/>
), el)

// While that's more code, now we can do this:
render((
  <Router render={(props) => (
    <AsyncProps {...props} render={(props) => (
      <RelayRoutes {...props} render={(props) => (
        <ScrollRestorer {...props} render={(props) => (
          <RoutingContext {...props} />
        )}/>
      )}/>
    )}/>
  )}/>
), el)

Tasks

  • Remove RoutingContext prop, no need for deprecation warnings, it's undocumented and everybody we've told to try it got an earful of "we aren't sure about this thing yet"
  • Add render prop to Router
  • Add a default prop that just renders a RoutingContext
  • Write a document about how to write a middleware component (should continue to send down the props in a method called render

Holy smokes that looks like a lot, it's really not too much, and we're feeling really good about it all.

@timdorr
Copy link
Member

timdorr commented Dec 5, 2015

Do you guys have some code to push so we can see where you're at and where we can help? You have the advantage of being in person, so your cycle times are much lower, but d be happy to help with any menial changes that need to be made.

@ryanflorence
Copy link
Member Author

I don't think @mjackson has internet on his flight home from London, I'll probably have some code to push by the end of my flight. And then others can poke the code and see if it works as we'd like it to.

@timdorr
Copy link
Member

timdorr commented Dec 5, 2015

Just some thoughts/feedback now that I've had a chance to read through all of this:

Remove default to hashHistory and require devs to provide one, should help with people not understanding that there's a better option, and allow devs to decrease the bundle size of their app

I think, for the sake of making the upgrade less painful for hash users, we should leave it in the repo, but not export it. Users that need it still have a one-liner to access it, but we also don't increase bundle size. It can be explicitly deprecated for removal in a 2.x release.

Add context.router that looks like this:

Could we add history as another context object property? I know we can get access to it via the singleton export, but that is convention over configuration. I'm pretty ambivalent about it, though. The only things I can think of needing access to history's API can be accomplished in other ways, so this may be a moot point.

const RouterMixin = {

Given how simple this is, I think we should bundle it into the library. Then I can throw @router at the top of my components to decorate in this.router really easily.

render((
  <Router render={(props) => (
    <AsyncProps {...props} render={(props) => (
      <RelayRoutes {...props} render={(props) => (
        <ScrollRestorer {...props} render={(props) => (
          <RoutingContext {...props} />
        )}/>
      )}/>
    )}/>
  )}/>
), el)```

I think that will actually end up looking like this (provided with a good compose implementation):

render((
  <Router render={compose(
    renderAsyncProps,
    renderRelayRoutes,
    renderScrollRestorer,
  )}>
), el)

You can just bump up the order of the render function and make it easy for them to be nested. This is up to the library to implement, so I would document it as such to establish the pattern and make it the norm for other libraries. Though it is hard to enforce...

Overall, this looks great. There's a trend towards increasing userland code with this route, but I don't think it's all that terrible and really only affects those doing more eccentric things with the router.

@taion
Copy link
Contributor

taion commented Dec 5, 2015

💯 on this, this is fantastic. Have a few minor comments/notes but will write them up later.

@ryanflorence
Copy link
Member Author

@timdorr

I think, for the sake of making the upgrade less painful

1.0.0 API will work in 1.1.0, so upgrading should be "drop-in but now you have warnings". So yeah, we'll still have a default but with a warning.

@ryanflorence
Copy link
Member Author

@timdorr

Could we add history as another context object property?

Nope, React 15 or 16 won't let us, reread the issue :P Also, history is not going to be router API anymore. If you need your history, your app will have the object it passed to router, just import it and use it wherever, you don't need it in the render tree.

@ryanflorence
Copy link
Member Author

@timdorr

Given how simple this is, I think we should bundle it into the library.

Another great reason not to have it in the package :P We've never wanted to be in the "mixins v. decorators" business and now that context is documented we don't have to be.

@timdorr
Copy link
Member

timdorr commented Dec 5, 2015

I meant having history as a property next to push and replace and all that jazz. But you're right, hence my ambivalence 😄

@ryanflorence
Copy link
Member Author

Though it is hard to enforce

I would personally do the component version not the compose version, and since the compose version can be built on top of the component version, we'd keep with our philosophy of "building the lower-level thing and letting apps get opinionated about [stuff]"

@ryanflorence
Copy link
Member Author

@timdorr

Overall, this looks great. There's a trend towards increasing userland code with this route, but I don't think it's all that terrible and really only affects those doing more eccentric things with the router.

:D 💃 That's exactly the goal, thanks for considering all of this. We'll do our best to get some code up for everybody to help us fix soon.

@bjrmatos
Copy link

bjrmatos commented Dec 5, 2015

Any love for #2186 in v1.1.0? :)

@timdorr
Copy link
Member

timdorr commented Dec 5, 2015

@bjrmatos There's some support work on history for that first. Then it can be implemented on router. But it's independent of the changes here.

@ryanflorence
Copy link
Member Author

@bjrmatos @timdorr I believe that's here isn't it?

@taion
Copy link
Contributor

taion commented Dec 5, 2015

Okay, got back to my computer. Some more detailed thoughts:

  • I really like the idea of exporting history singletons in the router library. This gives us a principled way to "hide" history as a dependency for most users, while not doing the weird-seeming thing of just re-exporting all history methods that we rejected in react-router should re-export necessary functions from the history project #2211. We should figure out the default queryKey setting or equivalent for hashHistory, though - maybe resolve State management in hash history history#163 first.
  • The context.router API looks fantastic and is a great simplification. I'm not a huge fan of the current context.location and won't be sad to see it go. I also like explicitly hiding the listen method, since now that you're in a React context, you can just receive props.
  • We noted that the Lifecycle example doesn't have the componentWillUnmount hooks, but that just brings up the point that they're not really necessary - we can just tear down "route leave" hooks when we leave the route, which is actually good enough for most use cases IMO.
  • I think props.render is a better API than the current props.RoutingContext. Relay root renderers are configured the same way, and it's a good, versatile pattern. My personal preference for userspace composition looks like https://github.com/rackt/react-router/blob/v1.0.0/modules/__tests__/Router-test.js#L286-L292. In this context it'd be something like:
<Router render={props => (
  <RelayWrapper {...props} />
    <SomeOtherWrapper {...props} />
      <RoutingContext {...props} />
    </SomeOtherWrapper>
  </RelayWrapper>
)} />

But as noted, this is all in userspace anyway, so users can do whatever they want.

Overall, I'm 100% behind this. This is really fantastic, @ryanflorence.

@timdorr
Copy link
Member

timdorr commented Dec 5, 2015

@ryanflorence Yes, but this is about moving that API, not necessarily changing its signature. We can certainly do it at the same time, but it's not a requirement for this issue to close.

@timdorr
Copy link
Member

timdorr commented Dec 5, 2015

@taion I like that form of render even more! I was in FP mode in my head, so I forgot about props.children. That reads a lot better without having to layer on additional functional concepts.

@taion
Copy link
Contributor

taion commented Dec 5, 2015

@timdorr

Yeah, the main thing is that "what users choose to do" shouldn't affect our choice of API here - we don't have any control over the patterns they use.

For me, I'm probably going to keep just exposing <RelayRouter> as my main entry point (since most people won't need to compose multiple "routing context wrappers"), so not even the users of react-router-relay will see any overt difference.

@taion
Copy link
Contributor

taion commented Dec 5, 2015

@ryanflorence

On terminology - for the pushable locations, do you like the term "location descriptor"? I have it written up as "location spec" in my in-progress docs for the corresponding changes on history on remix-run/history#168, but I don't really like the "location spec" term.

@timdorr
Copy link
Member

timdorr commented Dec 5, 2015

@taion The only gotcha is we can't enforce using children, so that kind of composition can break down if a render wrapper doesn't expect them. We should document whatever form we choose so that the pattern and use cases for it are known.

@timdorr
Copy link
Member

timdorr commented Dec 5, 2015

What about just calling it a capital L "Location"?

@knowbody
Copy link
Contributor

knowbody commented Jan 4, 2016

@zuta RC means no breaking changes, just bug fixes if any (see this).

@zuta
Copy link
Contributor

zuta commented Jan 4, 2016

@knowbody Thanks. Sorry for the noob question.

@minhtranite
Copy link

Please provide a document about how to write a middleware component.
If use multiple middleware, how to handle createElement when both middleware depend on it (like).

const createElement = (Component, props) => {
  // How to handle Middleware1Container and Middleware2Container
};
...
ReactDOM.render(
    <Router routes={routes}
      history={history}
      createElement={createElement}
      render={props => (
        <Middleware1 {...props} render={props => (
          <Middleware2 {...props} render={props => (
            <RouterContext {...props}/>
          )}/>
        )}/>
      )}/>,
    document.getElementById('app')
  );

@DanielHeath
Copy link

So, at the moment the match function gives you renderProps, but doesn't actually tell you which route matched.

It'd be really useful to be able to introspect the matched route and navigate to its parents (e.g. I set an extra title attribute on the route and use that to set the <title> for server rendering).

It's super hacky, awkward code at the moment because there isn't really a nice way to (eg) inherit a default title.

@timdorr
Copy link
Member

timdorr commented Jan 7, 2016

Keep in mind that a route is normally comprised of a nested set of routes. When we pass along this.props.route to a routed component, that's the route for that particular segment of the route tree that is active. The full array of routes is available on renderProps.routes.

You can also get access to renderProps.components to get the list of components to be rendered (not their instances, just the classes). You can store static props on a component to access by the server. I do this to colocate data fetching functions alongside my components, which I resolve into my Redux state before rendering. You could do the same thing to store a title static property that you can use to fill in that part of your page template.

@DanielHeath
Copy link

Right - it works nicely from the component.

The awkwardness in this situation comes from the router only rendering the <body> of the page.

Being able to get the routes that matched from renderProps would mean not having to re-jig everything to give the router control of the full page.

@DanielHeath
Copy link

I could also use a static property (global variables, yay) - except that that's a race condition waiting to happen on the server.

EG:

get request #1
set static title property to "request 1"
block to fetch data
get request #2
set static title property to "request 2"
block to fetch data
Serve response for request #1 with title "request 2"

@timdorr
Copy link
Member

timdorr commented Jan 7, 2016

Being able to get the routes that matched from renderProps would mean not having to re-jig everything to give the router control of the full page.

I think you mis-read. Those are available. They're at renderProps.routes.

@DanielHeath
Copy link

Ahh, I understand now. I was confused when inspecting it because the first one was the root route (so looked like a complete list of routes.

@vishaltelangre
Copy link
Contributor

This issue's description has following:

// v2.0.0
class RouteComponent extends React.Component {
  someHandler() {
    this.props.router.push(...)
  }
}

which should be:

// v2.0.0
class RouteComponent extends React.Component {
  someHandler() {
    this.context.router.push(...)
  }
}

Also, v2.0.0 Upgrade Guide doesn't seem to have instructions given here in Lifecycle Mixin with deep, non-route components section. I will send a PR if needed for the same.

Beside, I am really happy to see this terrific work on v2.0. A big "Hi, Five!" to the React Router team.

@mikestopcontinues
Copy link

Hey all, not sure if this is the right place for a v2 bug, but with [email protected] and [email protected] (installed as a dep for react-router), both browserHistory and hashHistory are being exported as undefined from /lib/index.js and their respective js files.

{ __esModule: true,
  // ...
  browserHistory: undefined,
  hashHistory: undefined,
  // ... 
}

@taion
Copy link
Contributor

taion commented Jan 22, 2016

They are in fact exported properly – double check your setup.

@mikestopcontinues
Copy link

OK, can you offer a baseline to work from? I was using node 4.0.0 before, but I just tried upgrading to node 5.5.0, npm 3.5.3. I did a clean sweep in a new project, npm cache clean && npm init -y && npm install --save [email protected], and the problem is the same. Not using babel for this test either.

@mmahalwy
Copy link

Any idea what should be done as a fix for scroll-behavior for the time being?

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

@mjackson just pushed history 2.0.0-rc3 (Thanks, Michael! 🤘), so I think we're all good here.

@ryanflorence Can you push out a 2.0.0-rc6 to suss out any final bugs? We can go 2.0.0 final soon afterwards 😄

@timoxley
Copy link

timoxley commented Feb 5, 2016

You probably shouldn't publish prereleases or rcs as the latest tag on npm, latest is usually treated as the latest stable version.

@timdorr
Copy link
Member

timdorr commented Feb 5, 2016

@timoxley That was fixed over here. It probably just needs to be applied to NPM as well.

@tleunen
Copy link

tleunen commented Feb 5, 2016

Testing the rc5 now and using the hashHistory, how are we supposed the disable the queryKey?

@timdorr
Copy link
Member

timdorr commented Feb 5, 2016

@coryhouse
Copy link
Contributor

Is there a publicly viewable version of the full docs updated for 2.0 public yet?

I'm aware of the 2.0 upgrade guide. Is that (and this thread) still the sole docs for 2.0?

@taion
Copy link
Contributor

taion commented Feb 7, 2016

The docs on master are for 2.0.

@dagatsoin
Copy link

Coming from the redux-router-react discussion and switch to react-router 2.0 only for now. My 2 cts experience for the repo: hard to be sure if the master is the V2. The npm version is confusing.

@timdorr
Copy link
Member

timdorr commented Feb 10, 2016

Guess we can close this... 😄

@timdorr timdorr closed this as completed Feb 10, 2016
@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
None yet
Projects
None yet
Development

No branches or pull requests