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

Implement basic lifecycle hooks #17

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

gaguirre
Copy link
Contributor

@gaguirre gaguirre commented Jul 2, 2017

Closes #14

This is the updated list of the already implemented lifecycle hooks:

  • componentWillMount
  • componentDidMount
  • actionWillDispatch
  • actionDidDispatch
  • stateWillChange
  • stateDidChange

API spec

It's important to well define the behavior for each one to avoid breaking the API in the future.

componentWillMount()

This hook receives no parameters and executes when the component will be mounted.
In this stage the state and reducers are not mounted yet, so no reducers can be called.

It should be called in the same order the Reduxables are instantiated: if we have a Child reduxable and a Parent reduxable who has a Child instance in its state, then the componentWillMount() will be called first for the Parent and the for the Child.

componentDidMount()

This hook receives no parameters and executes when the component is already mounted.
In this stage the state and reducers are already mounted, so any reducer can be called.

It should be called in the opposite direction than the componentWillMount: if we have a Child reduxable and a Parent reduxable who has a Child instance in its state, then the componentDidMount() will be called first for the Child and the for the Parent.

actionWillDispatch(action)

This hook receives an action as parameter and executes immediately before that action dispatch.

  • TODO: define if the received action is the action to be dispatched or a copy
  • TODO: define if the return value should be used for something like preventing the dispatch or returning the final action to dispatch

actionDidDispatch(action)

This hook receives an action as parameter and executes immediately after that action dispatch.

stateWillChange(newState)

This hook receives the newState as parameter and executes immediately before setting that newState.

  • TODO: define if the received newState is the state to be set or a copy.
  • TODO: define if the return value should be the final state to be set.

stateDidChange()

This hook receives no parameters and executes immediately after the state has changed.

@gaguirre gaguirre requested review from jpgarcia and ignacioola July 2, 2017 23:25
@gaguirre
Copy link
Contributor Author

gaguirre commented Jul 3, 2017

To properly implement componentWillMount we should avoid using the constructor to define the state/reducers.
Currently we've cases where we can't ensure the desired order (first parent, then child) like the following: the Child's componentWillMount executes first because the Parent need to call Child's constructor before calling its own constructor.

class Parent extends Reduxable {
  constructor() {
    super({ child: new Child() }, { someReducer: x => x })
  }
}

new Parent()
// will call the Child's `componentWillMount` before the Parent's one

We should consider change the API moving the initial state creation to a method like getInitialState(). This way internally (in the Reduxable constructor) we can ensure the desired order doing:

  1. Call the componentWillMount

In this step would execute the Parent's hook

  1. Call the getInitialState() and do all the stuff

In this step would execute all the children hook recursively

  1. Call the componentDidMount

The same case would look like

class Parent extends Reduxable {
  getInitialState() {
    return { 
      child: new Child() 
    } 
  }

  static reducers = { 
    someReducer: x => x  
  }
}

@jpgarcia @ignacioola I'd like to discuss with you this :)

@gaguirre gaguirre force-pushed the feature/lifecycle-hooks branch from c743a9e to af34041 Compare August 15, 2017 15:46
Copy link
Contributor

@ignacioola ignacioola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants