Refactor appender to use the render function instead of a component instance

[renatho]

Description

This deprecates the renderAppender prop in the InnerBlocks, introducing a new appender prop for this purpose. The difference is that the appender now is a rendered component instead of the component function/class.
If the appender is null, nothing is rendered.
If the appender is undefined the default is rendered.
More details here.

How has this been tested?

• Tested running the tests, and in a real project using the feature.

Types of changes

It's basically a refactor that should not impact in anything.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @renatho! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[talldan]

I don't think this would work if renderAppender used a class component. So it'd be a breaking change.

(edit: although two things - the renderAppender docs are wrong, and this PR makes it works as the docs suggest. It might also be possible to update this to work with class components).

[talldan]

I've updated the docs to match how it currently works - #29925.

[renatho]

I don't think this would work if renderAppender used a class component. So it'd be a breaking change.

Ah, that's true. I'm closing it, and thank you for updating the documentation!

EDIT: since we support the class too, I don't think it would be necessary the refactor. But if someone think it would be nice, I'd be happy to re-open this and add the support to classes, keeping the render prop option as the render prop pattern.

[renatho]

Reopening this PR since we don't have a way to pass custom props to the appender unless using a function. I made a refactor introducing a new boolean property renderAppenderAsFunction, so it keeps the backward compatibility, and we can use the render prop by setting it to true.

I thought of using a separate prop for the render prop, but probably it would get more confusing.

[renatho]

Reopening this PR since we don't have a way to pass custom props to the appender unless using a function. I made a refactor introducing a new boolean property renderAppenderAsFunction, so it keeps the backward compatibility, and we can use the render prop by setting it to true.

I thought of using a separate prop for the render prop, but probably it would get more confusing.

Hi @talldan! Have you seen these changes?

While that, we're using useCallback as workaround, as it's used in some cases in Gutenberg. But I don't think the useCallback for this would be the best solution because we're trusting that to not re-create the instance of the component.

[ellatrix]

If there's no issue with the current approach, is it worth adding an additional prop (public API) for this?

[renatho]

If there's no issue with the current approach, is it worth adding an additional prop (public API) for this?

I think the problem with the current approach is that in these cases we're trusting in the useCallback to create the component function that should be a static thing. As you said, it's not an issue now, but it could be in the future, depending on React improvements. 🤔 So if you think we could keep it as it is, and only fix it if we have a problem in the future, that's okay to me.

But since it doesn't seem a good practice, my opinion would be that we should introduce a solution for these cases (needing to use local props or states to create the appender). 😊 Some solution like this new prop would also make more transparent that the renderAppender is not a render prop.

[talldan]

I can see the shortcomings with renderAppender and personally I'm ok with moving on from it if there's no good alternative. While I don't really like render props as a pattern, the current implementation does seem to have been a source of bugs for some plugins and the naming causes confusion.

If there's a new prop, I think it'd be best to deprecate renderAppender.

renderAppenderAsFunction is a bit verbose. What are the alternatives names? Could it just be appender? Or renderBlockAppender?

[renatho]

I can see the shortcomings with renderAppender and personally I'm ok with moving on from it if there's no good alternative. While I don't really like render props as a pattern, the current implementation does seem to have been a source of bugs for some plugins and the naming causes confusion.

If there's a new prop, I think it'd be best to deprecate renderAppender.

renderAppenderAsFunction is a bit verbose. What are the alternatives names? Could it just be appender? Or renderBlockAppender?

Hey there!

Honestly, I suggested the render prop because the current prop naming (renderAppender).

But I think the render prop is not needed here for now because we don't send any prop for the component. It could be helpful for the future if we want add arguments coming from the BlockListAppender though.

So maybe for now we could just deprecate the property as suggested, and just send a rendered component as appender. Like:

const appender = <MyAppender { ...myProps } />

return <InnerBlocks appender={ appender } />

WDYT?

[renatho]

I implemented the new approach. I think deprecating the renderAppender and making the appender be the new prop, as suggested, made it much cleaner. =)

[ceyhun]

I've tested this on iOS and Android using Column and Group blocks and it's looking good. I just have a comment about possible loss of a previous performance improvement.

[renatho]

@talldan, I think after the mobile review, we're good to go ahead with this. WDYT?

[renatho]

Rebased to fix the new conflicts, and updated the deprecated versions! ;)