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

Pass prevState as argument to onLeave #3612

Closed
albertogasparin opened this issue Jul 7, 2016 · 4 comments
Closed

Pass prevState as argument to onLeave #3612

albertogasparin opened this issue Jul 7, 2016 · 4 comments
Labels

Comments

@albertogasparin
Copy link

This is a little enhancement I'm asking for. Currently, both onEnter and onChange have router state passed as an argument. That allows us doing stuff with router location and parameters. However, it is not the case for onLeave. Is there any particular reason?

Use case example: I'd like to handle socket.io room joining/leaving on the router.

<Route path="/" component={App}>
  <Route 
    path="/chat/:id" 
    component={Chat} 
    onEnter={(nextState) => { 
      joinChat(nextState.params.id); 
    }}
    onChange={(prevState, nextState) => { 
      leaveChat(prevState.params.id); 
      joinChat(nextState.params.id); 
    }}
    onLeave={(prevState) => { 
      leaveChat(prevState.params.id); 
    }}
  />
</Route>

I know this could be handled by the component itself, but I'm questioning why not passing the previous state to the onLeave callback, as it might be helpful in some situations.

@timdorr
Copy link
Member

timdorr commented Jul 7, 2016

I know this could be handled by the component itself

That's the reason right there. Make use of lifecycle methods as much as possible.

In your example, you can move the onChange up to your top level route instead.

@timdorr timdorr closed this as completed Jul 7, 2016
@taion
Copy link
Contributor

taion commented Jul 7, 2016

Is it weird that the API is inconsistent here? I do think it would feel more consistent if onLeave received prevState? It'd be a pretty easy change.

@taion
Copy link
Contributor

taion commented Jul 7, 2016

I'm inclined to add prevState there just for the sake of consistency. It's just really weird that onLeave doesn't have the signature you'd expect there.

One reason you may not want to just use lifecycle hooks there is (as with onChange) that you might be using named components.

@taion taion reopened this Jul 7, 2016
@taion taion added the feature label Jul 7, 2016
@timdorr
Copy link
Member

timdorr commented Jul 7, 2016

Sure, I'll whip up a PR now.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 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

3 participants