Skip to content
This repository was archived by the owner on Aug 23, 2022. It is now read-only.

Fixed deeply dynamic model support #815

Merged
merged 2 commits into from
Jun 7, 2017

Conversation

kabbi
Copy link
Contributor

@kabbi kabbi commented Jun 2, 2017

This fixed the case when the model dynamically changes in the middle of the chain - for example, you were using deep Fieldset hierarchy, and any of them had dynamic model
The problem is caused by the fact that resolveModel decorator, that is used internally to allow partial models, was caching model value upon mounting, thus preventing it from updating properly.
I've also removed several performance optimizations, as they were also preventing context updates. I'm open to any way this could be done better.

@davidkpiano
Copy link
Owner

I'll have to see how performance is affected with this. Can you elaborate more in how exactly the performance optimizations were preventing context updates? Maybe we can work around it without sacrifices.

@kabbi
Copy link
Contributor Author

kabbi commented Jun 2, 2017

The intermediate component you've created, DefaultConnectedControl, does not know anything about contexts that are used by resolveModel. So we can't check if it is updated, and restrictive shouldComponentUpdate implementation would block the context propagation, leaving Controls with an old model value.
Some kind of related discussion can be found here: facebook/react#2517.
I can declare contextTypes on DefaultConnectedContol though, and then check if they've updated in shouldComponentUpdate. But it seems hacky for thic component to know anything about context props used in resolveModel.

@davidkpiano
Copy link
Owner

I can declare contextTypes on DefaultConnectedContol though, and then check if they've updated in shouldComponentUpdate. But it seems hacky for thic component to know anything about context props used in resolveModel.

That might be an idea. The reason for resolveModel is that React-Redux's connect() doesn't know anything about context, unfortunately. If it did, that would fix the issue, as we can do resolved model-checking logic in there.

@davidkpiano davidkpiano merged commit e3c237e into davidkpiano:master Jun 7, 2017
@davidkpiano
Copy link
Owner

Looks good, thanks for adding the sCU logic back in!

@kabbi kabbi deleted the fix-resolve-dynamic-model branch June 8, 2017 09:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants