Adobe-Marketing-Cloud / target-atjs-extensions

Adobe Target extensions to at.js to support Single Page Application implementations
56 stars 29 forks source link

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

Open garrettmac opened 6 years ago

garrettmac commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

garrettmac commented 6 years ago

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