remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
53.04k stars 10.3k forks source link

[v4] onEnter and onChange hooks #3854

Closed olegafx closed 8 years ago

olegafx commented 8 years ago

How could we replace onEnter and onChange route hooks?

I found only 1 solution: additional <Match/> with a custom component. This component could use something like componentWillReceiveProps to emulate onChange. But what about onEnter? We have no lifecycle hooks that receive props on initial render.

Nice work, btw.

mjackson commented 8 years ago

Thanks :)

We have no lifecycle hooks that receive props on initial render.

We no longer have a "route lifecycle". Instead, since <Match> components are real components (not pseudo-components like <Route>s were) they get to participate in React's lifecycle, which means they can just use componentWillMount.

corydeppen commented 8 years ago

As someone who is very new to React, I'm trying to understand if the recommended approach is to have the component being rendered with Match (e.g. /items) implement componentWillMount and possibly set data on state instead of getting data passed via props. Or how might a table row containing a link to the item details be rendered with Match (e.g. /items/:id) - seems that wouldn't be able to have the item passed to it via props if I understand how the router works.

I appreciate any guidance.

mbrevda commented 8 years ago

Here is an example: https://reacttraining.com/react-router/web/example/auth-workflow

dtipson commented 7 years ago

Been looking over that example, but I think I'm not understanding how that works when it's asynchronous. If, say, /items/:id requires a network call to retrieve, how would the navigation transition get blocked until the response comes back and there's something new to display? I'd prefer not to have every page display have to have a componentWillMount empty state, if that makes sense.

It looks like there was an example that sort of went in documenting this kind of direction where the Router becomes slave to Redux, and navigations are all handled in there, but it's not part of the playground yet: https://github.com/ReactTraining/react-router/blob/7f6706dab4827afc1c26a58418f8ef8c8ab40125/website/examples/Redux.js

Is there a non-redux pattern?

MathieuLorber commented 7 years ago

Hello. I still don't understand how I'm supposed to replace onEnter with router v4. componentWill/DidMount seem "technical" hooks linked to dom lifecycle. With a route /view/:id, if I switch from /view/1 to /view/2, the same ViewComponent will be used, and componentDidMount() will not be called between the two different routes. If I'm fetching data using the :id, how I am suppose to do without an onEnter ? (besides this problem, I'm very seduced by the rest of the API ftm =). I particularly appreciate the login redirect solution, often wanted smthg like this with old single page apps)

MathieuLorber commented 7 years ago

I was thinking of using Route.render(), but I'm not sure to really understand when it can be called. It could result in useless fetching calls =/

pshrmn commented 7 years ago

@MathieuLorber componentWillReceiveProps

MathieuLorber commented 7 years ago

Sure, but the whole lifecycle of the component can be called for several reasons (any change in the app state). With componentWillReceiveProps, I have to check that the changed prop is the one which interests me for fetching. Or maybe by creating a dedicated wrapper component with no other props. Still, doesn't it sound a bit verbose ?

pshrmn commented 7 years ago

v4 is all about routes just being components. That means taking advantage of lifecycle methods.

componentWillMount() { // or componentDidMount
  fetch(this.props.match.params.id)
}

componentWillReceiveProps(nextProps) { // or componentDidUpdate
  if (nextProps.match.params.id !== this.props.match.params.id) {
    fetch(nextProps.match.params.id)
  }
}
MathieuLorber commented 7 years ago

Hmm I still must be missing something. Your proposition works in a component inside the Route, not in a component extending Route (componentWillReceiveProps is never called).

So I still need a dedicated wrapper component. And this solution is a lot more verbose + a lot more bug prone.

For the moment, using Route.render() is my best candidate. Code is there : https://gist.github.com/MathieuLorber/61ee584e375c41cd60038a11a9bf437b

Thanks for your help =) !

pshrmn commented 7 years ago

What is the purpose of your <MyRoute> component?

MathieuLorber commented 7 years ago

Well, I wanted to take advantage of lifecycle methods =]

pshrmn commented 7 years ago

The lifecycle methods should be on the component that is rendered by the <Route>. A <Route> is just there to match its path against the current location.pathname.

MathieuLorber commented 7 years ago

Ok.

And using Route.render() for that purpose sounds like a good idea or not ?

pshrmn commented 7 years ago

I'm afraid I don't understand the question. Can you ask over on Reactiflux? This has turned into a bit of a conversation which would be more appropriate to have on an actual chat app :smile:.

richhauck commented 7 years ago

I have a similar need to what @MathieuLorber had (thanks for posting your code, BTW). In React Router v3, I had a function that updated a store with location data on onChange() and onEnter() hooks. This saved me the trouble of writing redundant code in each Route Component's respective componentWillMount().

The best v4 equivalents I've found so far are to either make a separate component that only exists to listen to changes and update a store (envisioning something like the [withRouter example] (https://reacttraining.com/react-router/web/api/withRouter) ) or to do what @MathieuLorber has done.

oallouch commented 7 years ago

I put my onEnter functions next to my actions factories. This way, all data fetching is at the same place. So, for me, it's better to stick with v3.

bmanturner commented 7 years ago

Is this really something you wouldn't be willing to add to BrowserRouter? I think it's the only thing keeping us back.

FredericHeem commented 7 years ago

Unfortunately, onEnter is one of the reasons why my project is stuck with v3.

gvidon commented 7 years ago

I have a tree of deeply nested routes. Before doing transition to one of them I need to fetch the data, and behave just like Github does — change url while keeping current render state, show non-blocking progress bar line in the top and then when data fetched do the actual transition.

It was pretty simple with [onEnter], because being passed with callback it blocks transition, which freeze current render state.

But now I can't find the way to prevent transition in order to wait for some job to be done. component* life cycle methods don't work for me as routes are deeply nested and calling life cycle methods will mean previous render state was dropped and new render started even before some background job is done.

What am I doing wrong? Thanks!

gvidon commented 7 years ago

By the way, I'm using react-router with redux and the only way I can see so far is writing redux middleware for every app module which has components performing data fetching. Obviously I don't like it.

wzuo commented 7 years ago

I am using v4, and I need to redirect to index page when user is logged in, so I decided to create temporary (hopefully..) fix to it.

Snippet: https://gist.github.com/wzuo/e9c18132c94b5494a3156b2a2e6e57f6

onEnter function have to return redirect path if browser need to be redirected, otherwise have to return null. Also, feel free to modify it - I am getting expression based on state

Most magic happens on line 65, the rest is almost copied from Switch component

gvidon commented 7 years ago

@wzuo from what I see it would work for me if I had just single Switch. But in my situation it is like:

Route
  Switch
    Switch

Route

Route
  Switch
gvidon commented 7 years ago

I misunderstood how react-router-redux works, injecting middleware will not help. Clicking Link or NavLink will cause changing history and triggering routes. I still can't freeze rendered state.

gsantiago commented 7 years ago

React doesn't offer any lifecycle hook that is triggered once, right before the initial render.

How can I emulate the onEnter behavior with the lifecycle methods?

The solutions I have in my mind are too verbose, repetitious and ugly. I still can't see the advantage of removing onEnter and onChange hooks. They were so simple...

MathieuLorber commented 7 years ago

@gsantiago

React doesn't offer any lifecycle hook that is triggered once, right before the initial render.

It does actually, it's componentWillMount. But it's still not what you want. componentWillMount + componentWillReceiveProps will do the trick, but you indeed have to check the changes of matches.

hutber commented 7 years ago

So @MathieuLorber so as everything in react-router is now a component it means that we'll need to make the calls to our auth logic inside of componentWillMount + componentWillReceiveProps in each route's components basically?

oallouch commented 7 years ago

@jamiehutber ...or stick with v3 :)

hutber commented 7 years ago

Na, gotta move with the times :D I prefer it being on a component level. :p

MathieuLorber commented 7 years ago

@jamiehutber I'm not sure to understand, your auth logic works "the same". Yes, it has to be done in component lifecycle. But you still just need one component. Here is an example : https://gist.github.com/MathieuLorber/b9b257d306ef72bbd930cd609c7daa3f (I'm not a react-router contributor, it's just my suggestion)

craftzdog commented 7 years ago

I created a decorator that lets component check if authenticated on componentWillMount like:

// check-authenticated.js
// @flow
import React from 'react'
import PropTypes from 'prop-types'
import type { State } from '../states'

export default function checkAuthenticated (Component: Object) {
  class AuthenticatedRoute extends React.Component {
    static contextTypes = {
      store: PropTypes.object,
      router: PropTypes.object
    }
    componentWillMount () {
      const { store, router: { history, route } } = this.context
      const { session }: State = store.getState()
      if (session.userId === null) {
        const { location: { pathname } } = route
        history.replace(`/?redirect=${pathname}`)
      }
    }

    render () {
      return React.createElement(
        Component,
        { ...this.props }
      )
    }

  }

  return AuthenticatedRoute
}

You can add the decorator to auth-required pages:

// page.js
// @flow
import React, { Component } from 'react'
import checkAuthenticated from './check-authenticated'

@checkAuthenticated
export default class Page1 extends Component {
  render () {
    return (
      <h1>Hello</h1>
    )
  }
}
santaclauze commented 7 years ago

There is a neat example of an HoC to deal with the authorization avoiding the mutiplication of ComponentWillMount and ComponentWillReceiveProps:

https://medium.com/@francoisz/react-router-v4-unofficial-migration-guide-5a370b8905a

detj commented 7 years ago

With v4, it seems I don't have much alternative to onEnter, either using the render method which must return a null. Again, I am not able to justify why one would put a data loading or some other before side effect inside the render method. The other alternative is to use component lifecycle methods, but what if my components are stateless functional components and I do not intend to change them and introduce component lifecycle methods in them. I like the way v4 has embraced components in routing parlance, this has made React Router a whole lot more easier to get onboarded with IMO, but it feels there could be a better way?

Maybe I'm missing some point, please correct me If I'm wrong.

detj commented 7 years ago

@santaclauze The HOC approach is neat indeed. I guess, will have to stick to this going forward.

oallouch commented 7 years ago

I'm not worried. V3 works like a charm and @ryanflorence knows there are issues with async needs.

detj commented 7 years ago

@oallouch Okay. Good to know that. 👍

moraleslos commented 7 years ago

Been converting to v4 and this is the last issue I've encountered which I can't figure out a good fix for. I had one route with an onEnter implementation that essentially would check the url to see if there was a particular hash and key in it. If so that meant to redirect to another page since it was coming from an ad link. OnEnter was perfect for checking conditions like this before rendering a component.

The other suggestions about using componentWillMount and such is not good in our particular case since everything was built with stateless functional components. Hopefully something like onEnter will be put back in v4-- apparently it's useful in many different cases just going off of this thread.

dr3w commented 7 years ago

I ended up writing this HOC. Works fine for my particular needs

const withRouteOnEnter = callback => BaseComponent => {
  const routeOnEnterCallback = (props) => {
    if (callback && typeof callback === 'function') {
      callback(props)
    }
  }

  class routeOnEnterComponent extends React.Component {
    componentWillMount() {
      routeOnEnterCallback(this.props)
    }

    componentWillReceiveProps(nextProps) {
      // not 100% sure about using `locatoin.key` to distinguish between routes
      if (nextProps.location.key !== this.props.location.key) {
        routeOnEnterCallback(nextProps)
      }
    }

    render() {
      return <BaseComponent {...this.props} />
    }
  }

  return routeOnEnterComponent
}

...

const loadData = props => {
  fetchStuff(props.someId)
}

<Route path="/articles/:id" component={withRouteOnEnter(loadData)(ArticleViewContainer)} />
MathieuLorber commented 7 years ago

@dr3w this is very similar to what I'm doing. Suggesting a more generalist pattern in react-router documentation would probably be the best solution (I wanted to work on a PR in this direction since several weeks but could take the time for the moment =s)

kevr commented 7 years ago

Let me get this straight. You would rather force a user to manually check, as opposed to using the deduction logic that onEnter provides? It doesn't make much sense to me. If something is asynchronous, now, instead of being able to rely on the data being available, we are forced to use a ternary render, and also checking for the data properly every step of the way?

It sounds like more runtime work, and it sounds like we just went back in time. Moving with the "times" is not necessarily relevant, when this is not a widely accepted standard at this point. Leaving this feature out, for almost no reason, is pretty disturbing for back-compatibility and flexibility.

oallouch commented 7 years ago

@kevr it's really only a matter of async Route preparation. Unfortunately, it's ALWAYS async :)

stiff commented 7 years ago

One possible workaround for this is to use redux connect:

const mapDispatchToProps = (dispatch) => {

  /* this code executed once before rendering component */
  doSomePreparation().then((preparedData) => {
    dispatch(dataForComponentLoaded(preparedData));
  });

  return (dispatch) => ({});
}

export default connect(
  mapStateToProps,
  mapDispatchToProps
)(SomeComponent);
mjackson commented 7 years ago

Sorry everyone, I had to lock the conversation on this issue because it's going nowhere. The short story is your routes are just components now. Use them like any other component. The router is not a framework.