Skip to content
This repository has been archived by the owner on Feb 19, 2022. It is now read-only.

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

Open
go1dfish opened this issue May 8, 2018 · 13 comments
Open

Comments

@go1dfish
Copy link

go1dfish commented May 8, 2018

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
Copy link

can you fiddle this?

@go1dfish
Copy link
Author

go1dfish commented May 18, 2018

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
Copy link

bingomanatee commented May 18, 2018 via email

@bingomanatee
Copy link

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
Copy link
Author

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.

@go1dfish go1dfish changed the title Freactal doesn't work well with nested/tree structures. Freactal doesn't work well with recursive state provider components. May 19, 2018
@agurtovoy
Copy link
Contributor

@go1dfish It's a bug, I fixed this in our fork: textpress@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).

@bingomanatee
Copy link

bingomanatee commented Jun 9, 2018 via email

@go1dfish
Copy link
Author

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

@agurtovoy
Copy link
Contributor

@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
Copy link
Contributor

divmain commented Jul 6, 2018

@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
Copy link
Contributor

divmain commented Jul 6, 2018

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

@agurtovoy
Copy link
Contributor

@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
Copy link
Contributor

@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?

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

No branches or pull requests

4 participants