FormidableLabs / freactal

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

Freactal doesn't work well with recursive state provider components. #104

Open go1dfish opened 6 years ago

go1dfish commented 6 years ago

Consider a rendering of nested comments.

Say you build up a state provider "withReplies" that takes an id prop and fetches replies to put in the state.

This withReplies approach works great, until you attempt to nest these components, as a error is thrown

Uncaught TypeError: Cannot redefine property: <piece of state like the id goes here>

Is there a way I can nest fractal state providers such that the inner most states replace the outer most states in their scope?

bingomanatee commented 6 years ago

can you fiddle this?

go1dfish commented 6 years ago

Is there a base fiddle somewhere? ideally with react. A more descriptive title should probably indicate recursive so I'll update that.

The pattern that I had a failure with here is: (leaving out the code to actually figure out who the children are)

const personProvider = provideState({
  initialState: ({ person }) => ({ personId: person }),
  /// effects to fetch children etc here
  computed: { personsChildren }
});

const Person = personProvider(injectState(({ state: { name, personsChildren } }) => (
  <li>
    {name}
    <ul>{personsChildren.map(id => <Person person={id} />)}</ul>
  </li>
))));

I originally wanted to use freactal to manage the state here:

https://github.com/notabugio/notabug/blob/master/src/components/notabug/Listing.js

But was unable to as Listing's are nested inside of each other for comment trees.

bingomanatee commented 6 years ago

Why would initial state have arguments?

Sent from my iPhone

On May 18, 2018, at 1:43 PM, go1dfish notifications@github.com wrote:

Is there a base fiddle somewhere? ideally with react. A more descriptive title should probably indicate recursive so I'll update that.

The pattern that I had a failure with here is: (leaving out the code to actually figure out who the children are)

const personProvider = provideState({ initialState: ({ id }) => ({ personId: id }), /// effects to fetch children etc here computed: { personsChildren } });

const Person = personProvider(injectState(({ state: { name, personsChildren } }) => (

  • {name}
      {personsChildren.forEach(id => )}
  • )))); I originally wanted to use freactal to manage the state here:

    https://github.com/notabugio/notabug/blob/master/src/components/notabug/Listing.js

    But was unable to as Listing's are nested inside of each other for comment trees.

    — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

    bingomanatee commented 6 years ago

    also in general - complex/nested things are pretty messy in many scenarios. I'd recommend flattening out your tree into an array of objects with id's and parent_ids as static values, and assemble them within a view context. You may find this a more manageable store than a deep nested object.

    go1dfish commented 6 years ago

    Regardless of the data structure sometimes a component will contain another instance of itself and need to manage state,

    The first argument to initialState is props passed into the wrapped component.

    agurtovoy commented 6 years ago

    @go1dfish It's a bug, I fixed this in our fork: https://github.com/textpress/freactal/commit/4db4e42b172ae6f32a6c58a0fb33f5ddc20bd625

    Note that it's triggered by the presence of computed properties (might be useful for working around this if you don't want to mess with forking).

    bingomanatee commented 6 years ago

    I’ve found computed properties buggy; fortunately they’re easy to externalize with helper functions that take in state as a parameter

    Sent from my iPhone

    On Jun 8, 2018, at 8:40 PM, Aleksey Gurtovoy notifications@github.com wrote:

    @go1dfish It's a bug, I fixed this in our fork: textpress/freactal@4db4e42

    Note that it's triggered by the presence of computed properties (might be useful for working around this if you don't want to mess with forking).

    — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

    go1dfish commented 6 years ago

    @agurtovoy good to know, any reason you haven't submitted your branch as a PR?

    agurtovoy commented 6 years ago

    @go1dfish Lack of time at the moment, basically. Our fork has more changes, some of which are experimental. I should have made a fix for this in a standalone branch, but was in a time crunch. Also, my earlier PR is still waiting to be merged ;)

    divmain commented 6 years ago

    @agurtovoy, if you don't have time for a PR, I may go spelunking through your fork and cherry-pick the fixes you've introduced. Interested in some of your experiments, as well.

    divmain commented 6 years ago

    Also, sorry again for going incommunicado on freactal for awhile. Job change can do that :)

    agurtovoy commented 6 years ago

    @divmain Please feel free to cherry-pick the fixes. You might want to fix this one differently, too; I went for the Set because it seemed like the easiest way to de-dupe the keys w/o introducing a dependency, but technically is is a dependency if you need to support older browsers.

    No worries about the delay, and thanks for open sourcing this in the first place!

    agurtovoy commented 6 years ago

    @divmain I finally have some time to devote to this; what's freactal's browser support policy? Should I just create a pull request with my original fix (with the Set dependency), or do you want me to roll a local unique impl?