[Navigation] Consider making Navigation Link a dynamic block

[talldan]

Background
The Navigation Block is currently a bit unusual in that it's a dynamic block with Inner Blocks. The only allowed inner block is Navigation Link. Currently, Navigation Link's rendering is all handled by the parent Navigation Block in a really big for loop:
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/navigation/index.php#L205-L271

This is also unusual since most blocks are responsible for rendering themselves. I don't think Navigation having this responsibility over Navigation Link offers a particularly good separation of concerns, so I'd like to make this suggestion to improve the situation.

How can this be improved?
The Navigation Link could itself be registered as a dynamic block. That for loop linked to above could be replaced with the following code:

$inner_blocks_html = implode( array_map( 'render_block', $block['innerBlocks'] ) );

And the Navigation Link could then implement its own render callback, taking responsibility over its own content and more closely mirroring the way static blocks are implemented.

Eventually this could also lead to a more standard approach for rendering innerBlocks within a dynamic block, e.g. a render_inner_blocks function.

Benefits

In addition to the code quality improvements, if there was ever a requirement to introduce other child blocks types to the Navigation Block, this kind of implementation would make it much easier.

Blockers

There would need to be quite a bit of refactoring of the Navigation Block. Over time, because there hasn't been a separation of concerns, the functionality of Navigation and Navigation Link have become intertwined. Setting font sizes and colours on the Navigation Block changes the markup of Navigation Link. The way this works would have to be rethought.

Navigation Link also itself has InnerBlocks, so it would need to use the same approach to rendering its own inner blocks. I don't think this should be an issue.

[talldan]

cc: @getdave, @retrofox, @obenland, @draganescu. Pinging you for thoughts. Anything else I haven't thought of that would make this difficult?

[talldan]

Related - #17194 also discussed some approaches for rendering the nav block.

[draganescu]

I think this separation of concerns makes a lot of sense. Also, it kinda is the moment to consider it thoroughly since the more we advance on this block the harder will become to dismantle all the logic in its rendering.

However, if we decide that rendering the navigation should be done in a way that results in the same markup and extensibility as the current menus (see #17194 for details) then having InnerBlocks render themselves could make it potentially harder: a Walker expects a tree and it makes the HTML out of that.

[mtias]

Currently, Navigation Link's rendering is all handled by the parent Navigation Block in a really big for loop

If a block uses inner-blocks it should have atomic rendering. That's where we want to move with things like Post Query and all its child blocks, as it'd allow composition to happen server-side. It won't reach isomorphic rendering, but things will become a lot more straightforward as each child block is a node with its own rendering.

[getdave]

This makes sense to me.

When I first saw this Issue title I thought it was referring to the scenario whereby links to Pages are added but are not dynamically updated if the Page slug changes.

I wonder whether this change will help make it easier to implement functionality whereby the link gets updated in response to updates to the original resource?

[mtias]

@getdave the problem for that is the url should not be stored on the block attributes, it should be created at render time.

[draganescu]

@mtias by Post Query you mean the work in #20106? Could you elaborate more on how Post Query would allow composition on the server?

[getdave]

@getdave the problem for that is the url should not be stored on the block attributes, it should be created at render time.

Agreed for any links that are referencing entities (eg: Posts) we should store a reference to that entity not the entity itself.

We do also handle direct links and those urls should be stored in attributes right?

[mtias]

We do also handle direct links and those urls should be stored in attributes right?

Yes