[Fiber] disableNewFiberFeatures bugfix: host component children arrays #8797
Conversation
|
Doesn't this mean that fragments are not disabled? |
|
No there's an invariant for composites. Here's the test: https://github.com/acdlite/react/blob/ff6347a270605653c1859e58ad9b30cc557ee1a4/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js#L1027 |
|
Also, what about the What exactly does this branch do? |
|
Haha you're right. Ok I'm definitely going to switch this flag to do be the default so we run all the tests against it. |
|
Updated. I moved the top-level invariant to the DOM renderer because the requirements vary too much. E.g. we need to allow |
| const instance = returnFiber.stateNode; | ||
| if (instance.render._isMockFunction) { | ||
| // We allow auto-mocks to proceed as if they're | ||
| // returning null. |
There was a problem hiding this comment.
What you're really saying here is that they're allowed to return anything (including strings, arrays etc.)
There was a problem hiding this comment.
Good point. Looks like Stack only accepts undefined. I'll add a test.
|
@sebmarkbage @spicyj Ping |
| // returning null. | ||
| break; | ||
| } | ||
| switch (returnFiber.tag) { |
There was a problem hiding this comment.
Seems unnecessary to fork this whole function now. Can't you just add this conditional down below behind a smaller if (ReactFeatureFlags.disableNewFiberFeatures) { instead of forking the whole function?
There was a problem hiding this comment.
Forking the entire function was the only way I could get it down to a single check. But you're right that there's a lot of overlap and it's weird to fork the entire thing. I split it out into three checks instead.
This exposed a few more cases that I missed.
Treat them as if they return null
Easier to follow this way, and less chance the two paths will get out of sync.
When
disableNewFiberFeatureswas enabled, host component children arrays were treated as empty.