Components: Improve empty elements filters in Slot implementation

[gziolo]

Description

Raised by @aduth in #5952 (comment):

Why do we filter here?

To get rid of empty elements. Some fills render nothing based on Redux state. If you want to prevent rendering a slot wrapper it it has no fill which render something this is the way to go. In retrospective, I think this could be also done outside. Should we move it out or improve here and add some comments?

Comments would be good.
It was also strange to me that we filter only if the children is passed as a function, but not otherwise.
Also, filter( Boolean ) doesn't sound like a sufficient way to imitate React's treatment of "empty". For example, a child of 0 would be omitted here, but is a valid / non-empty child to render.

This PR adds isEmptyElement helper method and adds proper inline documentation explaining why it is used with fills.

How has this been tested?

npm test

Types of changes

Refactoring.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[gziolo]

We use it only here, if there is going to be another use case, we could think to move it @wordpress/element.

[talldan]

Looks like a nice improvement. The only edge-case I can think of is isEmptyElement( new String( '' ) ), which I think would return false. It'd be pretty easy to fix that using the same approach used with arrays.

Would also be great to add jsdocs for isEmptyElement

[gziolo]

Looks like a nice improvement. The only edge-case I can think of is isEmptyElement( new String( '' ) ), which I think would return false. It'd be pretty easy to fix that using the same approach used with arrays.

Correct, I updated the way we validate strings to accommodate that. I skipped a similar check for new Boolean( false ) because it ain't as easy. In general, both should never happen 😃

Would also be great to add jsdocs for isEmptyElement

Added.

[talldan]

Great - I'm happy with those changes.

In general, both should never happen

🤞 hopefully!

[aduth]

This pierces the abstraction of @wordpress/element. Should not this have been added there instead?

[aduth]

This pierces the abstraction of @wordpress/element. Should not this have been added there instead?

I suppose it's okay to allow this to be deferred as it relates to a 3.8 RC, only because it's not part of a public API.

[gziolo]

I suppose it's okay to allow this to be deferred as it relates to a 3.8 RC, only because it's not part of a public API.

I left my own comment about it #9371 (comment) where I assumed we don't want to expose it in @wordpress/element until we have another use case for it.

[aduth]

In that case I'd rather just flag it as experimental.

My main issue is that, as far as anything in components is concerned, there should be zero awareness of what value an element holds. There is no commitment guaranteed by @wordpress/element that this shape is to remain, and will be hard to account for if ever changed. And we'd do well to avoid future headaches of plugin authors who follow similar practices to see how to accommodate this need by "solving" it internally for our own usage (as otherwise, they'll follow the same pattern and similarly face breakage whenever element structure changes).

[gziolo]

Opened #9681 with the proposed change. Let's continue there.