Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Target Components children will not update. Shouldn't the users handle the shouldComponentUpdate() logic and not the Target Component? #37

Open
garrettmac opened this issue Nov 1, 2017 · 4 comments

Comments

@garrettmac
Copy link

Correct me if Im misunderstanding something but this plugin shouldn't be worried about shouldComponentUpdate() returning false in line 37 of the component for it's children. This just prevents it's child nodes line 15 of the component from rerender and has no bearing on the Parent <Target> node itself right?

Take the following:

<App>
       <div>
              <MyBody>
                        <Target>
                                <UsersWrappedContent>
                                                <MoreUserChangingContent/>
                                </UsersWrappedContent>
                        </Target>
              </MyBody>
       </div> 
</App>

According to the react docs React will replace the first node that it detects change.

React will tear down the old tree and build the new tree from scratch.

When tearing down a tree, old DOM nodes are destroyed.

<App>           <----So it will check the root node.  Has it been updated? No. Next!
       <div>           <---- Has it been updated? No. Next!
              <MyBody>           <---- This one? No. Next!
                        <Target>            <---- This one? Yes. so just replace it's children while maintaining state across render
                                <UsersWrappedContent>
                                                <MoreUserChangingContent/>
                                </UsersWrappedContent>
                        </Target>
              </MyBody>
       </div> 
</App>

Further:

When a component updates, the instance stays the same, so that state is maintained across renders. React updates the props of the underlying component instance to match the new element
Any components below the root will also get unmounted and have their state destroyed.

So if the component is maintained across renders is there anything i'm missing as to why you're setting shouldComponentUpdate() like that?

(I also made this example pen to illustrate what shouldComponentUpdate() is doing if it helps)

@XDex
Copy link
Collaborator

XDex commented Nov 2, 2017

@garrettmac Right, the things that you mention above stand true for cases when a child component manages its state internally via setState() calls. However, another use case is when a child update is triggered by a parent passing new props to it (such as in Flux/Redux flows), and in this case, making shouldComponentUpdate() return false stops the propagation of props from parents to children, and thus the child components are never re-rendered.
We need to do this, because once at.js makes some modifications to the React-generated DOM we need these changes to be persisted (and not overwritten by a subsequent re-render when some child component updates), and the most straightforward way to do it is by disabling re-rendering of the children that the Target component wraps..

@garrettmac
Copy link
Author

@XDex is there anyway work around I can do to to have a stateful component be a default component outside of the <Target> wrapper?

@XDex
Copy link
Collaborator

XDex commented Nov 3, 2017

@garrettmac Sorry, I'm not following, could you please provide some more context and clarify what you mean by default and fallback components?

@garrettmac
Copy link
Author

@XDex if there are no offers to load or getOffer or applyOffer returns an error I don't want to loose my ability to manage state.

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

No branches or pull requests

2 participants