State management refactor #42
Conversation
emcelroy
force-pushed
the
state-management-refactor
branch
from
850a240
to
4e823e6
Compare
emcelroy
force-pushed
the
state-management-refactor
branch
from
4e823e6
to
af75d30
Compare
| const rootCollection = useMemo(() => { | ||
| return collections.find((c: ICollection) => !c.parent); | ||
| }, [collections]); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [collections, collections.length]); |
There was a problem hiding this comment.
Probably not now, but this would be better moved into the model. So to support that we'd have to make the model not just be a types.map(... but instead be a types.model(... so it could have a view of rootCollection.
In theory the number of collections might be constant even though the root collection has changed. I don't know collections well enough to know how likely this is.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [collections, collections.length]); |
There was a problem hiding this comment.
This is another example of something that would be better as a view on the model. But again it probably doesn't make sense to change that in this PR unless there are bugs caused by the current approach.
src/hooks/useCodapState.tsx
Outdated
| const newCollectionModels = colls.map((coll: ICollection) => { | ||
| const { areParentChildLinksConfigured, attrs, caseName, cases: _cases, childAttrName, | ||
| collapseChildren, guid, id, name, parent, title, type } = coll; | ||
| return { | ||
| areParentChildLinksConfigured, attrs, caseName, cases: _cases, childAttrName, | ||
| collapseChildren, guid, id, name, parent, title, type | ||
| }; | ||
| }); | ||
| applySnapshot(collections, newCollectionModels); |
There was a problem hiding this comment.
This looks like the same code that is in updateCollections, can you consolidate it?
@scytacki Thanks. I consolidated the duplicate code. I also had a go at updating the collections model like I think you were suggesting. Do those changes make sense? They appear to work fine, though I find the structure a little awkward. It seems a bit weird to have a |
To be more analogous with CODAP itself, |
@kswenson Thanks. That makes a lot of sense. I spent some time trying to make that work, but it doesn't seem as straightforward as you might expect, and it's still not clear to me how much more time it'd take to work that out. So I'd like to either stick with the current model changes or undo them and save that work for later like Scott originally suggested, if that sounds reasonable to you and @scytacki? |
We've added MobX State Tree to help with state management.