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

Provide accessible data #17

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

Provide accessible data #17

wants to merge 4 commits into from

Conversation

autarc
Copy link

@autarc autarc commented Feb 11, 2016

This PR consist of 3 changes which should ease the integration with redux-falcor (+ react) by reading the data from the falcor cache:

  1. reduxFalcor is higher order component which returns a wrapping function instead of the component itself, this matches the common interface know from react-redux and allows better composition (e.g. via decorators)
  2. fetchFalcorDeps got removed as it merely subscribes after componentDidMount and could be defined manually if required, moreover it emphasize the idea of using independent fetch approaches with universal support
  3. updateFalcorCache is the update action which is now exported to set the data directly

Checkout the updated example to for more details.

@ekosz
Copy link
Owner

ekosz commented Feb 12, 2016

Hey @autarc thanks for the PR a few thoughts:

  1. While I think having reduxFalcor work as a higher order component is great, I don't want to break the existing API. How about we do something like react-css-modules which allows for both APIs?
  2. Removing fetchFalcorDeps is pretty much a no go in my eyes, but would love to hear more about your decision process here. For me, fetchFalcorDeps is the main point of redux-falcor. For example I can define a dependency for a component on needing the users.length path. Then if any other action is made that invalidates that path, redux-falcor will automatically refetch that path for me and rerender the component. The rewrite of 3.0 was done to specifically add this.
  3. Exporting the action is totally cool with me!

What do you think?

@autarc
Copy link
Author

autarc commented Feb 13, 2016

Glad to share the intentions:

  1. Sure, it could probably check for the parameters passed and invoke the curried function if they are not used. The idea is just that its compatible to to higher order components (e.g. connect from react-redux etc.) and offer the same flexibility for composition.
  2. While providing a simple interface to load the falcor data sounds promising at first, the problem lies in the way to achieve that. Your implementation of fetchFalcorDeps will just be checked and called during the componentDidMount lifecycle hook at the client. This causes duplication of code actually to retrieve and insert the data in an universal setup (which prerenders components at the server). Decoupling this by using alternatives like redial helps to prevent the issue and separates the concerns. See following example:
@reduxFalcor()
@provideHooks({
  fetch ({ dispatch, falcor }) {
    return falcor.get('....').then(() => {
      return dispatch(updateFalcorCache(falcor.getCace())
    })
  }
})
export default Example extends Component {
  render(){
    const data = this.props.falcor.getCache()
    console.log(data)
  }
}

3.Great, this provides the opportunity to keep them controlled and set the store data manually .

@pickhardt
Copy link
Collaborator

Hi @autarc, what was the purpose of renaming update to updateFalcorCache?

Also, would my proposal in #22 help instead? It would allow passing your own fetchFalcorDeps as an optional parameter, which defaults to the one implemented already.

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.

3 participants