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

[v4] onEnter and onChange hooks #3854

Closed
olegafx opened this issue Sep 14, 2016 · 43 comments

Comments

@olegafx
Copy link
Contributor

commented Sep 14, 2016

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

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

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.

@mjackson mjackson closed this Sep 14, 2016

@corydeppen

This comment has been minimized.

Copy link

commented Sep 21, 2016

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

This comment has been minimized.

Copy link

commented Sep 22, 2016

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

@dtipson

This comment has been minimized.

Copy link

commented Nov 18, 2016

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

This comment has been minimized.

Copy link

commented Feb 13, 2017

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

This comment has been minimized.

Copy link

commented Feb 13, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2017

@MathieuLorber componentWillReceiveProps

@MathieuLorber

This comment has been minimized.

Copy link

commented Feb 13, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2017

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

This comment has been minimized.

Copy link

commented Feb 13, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2017

What is the purpose of your <MyRoute> component?

@MathieuLorber

This comment has been minimized.

Copy link

commented Feb 13, 2017

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

@pshrmn

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2017

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

This comment has been minimized.

Copy link

commented Feb 13, 2017

Ok.

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

@pshrmn

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2017

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

@richhauck

This comment has been minimized.

Copy link

commented Mar 14, 2017

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

This comment has been minimized.

Copy link

commented Mar 15, 2017

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

This comment has been minimized.

Copy link

commented Mar 22, 2017

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

@FredericHeem

This comment has been minimized.

Copy link

commented Mar 22, 2017

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

@gvidon

This comment has been minimized.

Copy link

commented Mar 23, 2017

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

This comment has been minimized.

Copy link

commented Mar 23, 2017

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

This comment has been minimized.

Copy link

commented Mar 23, 2017

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

This comment has been minimized.

Copy link

commented Mar 23, 2017

@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

This comment has been minimized.

Copy link

commented Mar 23, 2017

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

This comment has been minimized.

Copy link

commented Apr 7, 2017

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

This comment has been minimized.

Copy link

commented Apr 7, 2017

@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

This comment has been minimized.

Copy link

commented Apr 23, 2017

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

This comment has been minimized.

Copy link

commented Apr 23, 2017

@jamiehutber ...or stick with v3 :)

@hutber

This comment has been minimized.

Copy link

commented Apr 23, 2017

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

@MathieuLorber

This comment has been minimized.

Copy link

commented Apr 25, 2017

@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

This comment has been minimized.

Copy link

commented May 14, 2017

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

This comment has been minimized.

Copy link

commented Jun 4, 2017

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

This comment has been minimized.

Copy link

commented Jun 18, 2017

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

This comment has been minimized.

Copy link

commented Jun 18, 2017

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

@oallouch

This comment has been minimized.

Copy link

commented Jun 19, 2017

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

@detj

This comment has been minimized.

Copy link

commented Jun 19, 2017

@oallouch Okay. Good to know that. 👍

@moraleslos

This comment has been minimized.

Copy link

commented Jun 22, 2017

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

This comment has been minimized.

Copy link

commented Jun 27, 2017

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

This comment has been minimized.

Copy link

commented Jun 27, 2017

@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

This comment has been minimized.

Copy link

commented Jul 3, 2017

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

This comment has been minimized.

Copy link

commented Jul 4, 2017

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

@stiff

This comment has been minimized.

Copy link

commented Jul 16, 2017

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

@ReactTraining ReactTraining locked and limited conversation to collaborators Jul 17, 2017

@mjackson

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.