facebook / componentkit

A React-inspired view framework for iOS.
http://www.componentkit.org/
Other
5.76k stars 587 forks source link

Allow CKFlexboxBuilder to render children without initial child #956

Closed kparentfb closed 4 years ago

kparentfb commented 4 years ago

I found that CKFlexboxComponentBuilder() will never render its children if it's not initially constructed with an explicit child. For example:

auto flexbox = CKFlexboxComponentBuilder();
flexbox.child(componentA);
flexbox.build(); // componentA will not render
auto flexbox = CKFlexboxComponentBuilder().child(nil);
flexbox.child(componentA);
flexbox.build(); // componentA renders

I suspect it’s due to the logic here: https://fburl.com/diffusion/is68e3zp

Since hasActiveChild is false in the absence of an initial child prop, the new child isn’t pushed back, which prevents hasActiveChild from correctly recomputing, and no new children will ever be pushed onto the _children vector.

Andrey-Mishanin commented 4 years ago

This is happening because you've split the builder setup in multiple statements. As you might have noticed, the child() method returns a slightly different type (with the corresponding bit set in the bitmap), and that information is later used in the build() method to decide whether we need to add the current child or not. In your case setting this bit is not reflected in the bitmap that build() sees, so the child is not rendered.

In general, we advise against storing builders in variables exactly because it can cause bugs like this. If you have to do it, make sure you always reassign the builder variable, so this becomes a compile-time error instead of a runtime bug. (We could forbid storing builders in variables by removing the copy constructor but that's not currently possible for other uninteresting reasons.)

kparentfb commented 4 years ago

Thanks for the detailed reply @Andrey-Mishanin! This makes sense to me, I think I've even run into the builder's typing issue elsewhere.