FormidableLabs / freactal

Clean and robust state management for React and React-like libs.
MIT License
1.65k stars 46 forks source link

Making an async call in effects.initialize with nested container components leads to multiple service calls #47

Closed niki4810 closed 7 years ago

niki4810 commented 7 years ago

Hi @divmain,

Thanks for freactal I really love how it composes things. I have been playing with it for past few days and wanted your opinion on few things I've encountered. These may very well be an issue with how I am using the library.

I created a simple client-side rendered app with two state container components (i.e. components wrapped with provideState one at main level and the second one is nested within parent).

Here is the gist with complete code: https://gist.github.com/niki4810/57f9ca785ccfe00c9789b16668e4c6a4

These are the issues I am noticing: 1) Multiple fetch requests which calling service from effects.initialize: I am using the root containers, effects.initialize to make an fetch call to initialize the state data. I noticed that when I do this, this results in multiple fetch requests. I don't notice the same behavior when I remove the nested container component.

2) If initialState returns empty object, the state does not get's merged when server returns data When my root container component returns an empty object as initial state as show below

initialState: () => {
    return { };
  },
  effects: {
    getItemDetails: effects => fetch("http://output.jsbin.com/nuxodel.json")
    .then(result => result.json())
    .then(data => state => Object.assign({}, state, data)),
  },

If I later on call effects.getItemDetails call to my data providers (for e.g. here my service : http://output.jsbin.com/nuxodel.json), the state does not get merged correctly. Am I doing something wrong ?

Please let me know if you need further code samples. Looking forward for you feedback.

Thanks, Nikhilesh

jgoux commented 7 years ago

Same issue here. o/

divmain commented 7 years ago

This is definitely a bug. Thank you for the awesome repro. I'm on it!

Jascha-Sundaresan commented 7 years ago

Just wanted to confirm I've come across this bug recently as well.

It's happening because line 33 of provide.js is calling this.effects.initialize if it's present, but it's necessarily present in all children components.

An easier fix than the one proposed by @niki4810 is to overwrite effects.initialize in the child component, so when it's called, the parent's async call isn't fired.

Overwriting w/ a simple initializer: update({}) does the trick

Jascha-Sundaresan commented 7 years ago

Here's a PR that explicitly excludes a parent's initialize effect when setting a child component's effects.

divmain commented 7 years ago

Fixed in PR and released as v1.1.1.