Add @wordpress/menus package to better share code between nav block and nav editor

[getdave]

What problem does this address?

Currently the Navigation block and the Navigation editor screen are separate codebases in separate packages.

In one way this makes sense as it follows the established pattern in Gutenberg. Yet given that both features share a similar set of task requirements (eg: handling classic menus, converting to blocks...etc), I have observed an increasing amount of duplication between the two code bases.

This is causing problems for

• maintenance.
• consistency between editor and block.
• overhead/friction for the development of new features.

To mitigate these problems, I am proposing the creation of a @wordpress/menus package to share code between Navigation editor and block.

But first, allow me to explain why this is necessary.

Why is duplication a problem?

As @gwwar suggests, code duplication is generally a good first step to avoid tight coupling when we're still exploring a problem space.

However, over time as these duplications grow in quantity and complexity it becomes increasingly difficult to avoid drift. This very quickly leads to bugs and regressions being easily introduced. For example, new contributors come in to fix bug but don't realize that duplicate code exists in other places (indeed even seasoned contributors are capable of doing this!).

Placing too much reliance on humans to keep track of a large amount of duplicate code is a recipe for an unstable codebase. Moreover, it also places an additional burden on developers when authoring and/or reviewing code as this must now be performed in several locations thereby increasing the complexity of any given task and leading to unnecessary cognitive load. Again this leads to mistakes being made.

What examples do we have of these problems occurring?

Here is a basic example of a fix for a regression that occurred when a refactoring effort on the Navigation block was not duplicated into the Navigation editor. Both the author and the reviewer of the PR which originally introduced the regression are very familiar with the block and editor yet both still managed to forget about the need to duplicate the code.

In this instance, the knock-on effect was minor (Post Tag menu items were not being converted into their block equivalents in the Navigation block) but a similar error could easily have more impactful consequences.

You will also note the same PR also corrected other minor inconsistencies.

This is just a single example of many such regressions and one which I believe illustrates the type of problems that occur when complex and rapidly evolving code is duplicated.

What duplication do we currently have?

Let's review the key and most impactful areas where the current duplication exists:

• Similar (but yet unintentionally divergent) code which converts menu items to core/navigation-link blocks. Examples:
  • Editor
  • Block.
• Code for fetching Menus and Menu Items. Examples:
  • Block.
  • Editor.

The example above are both relatively complex and involved pieces of code. In addition, they both require the developer to maintain a (in brain) memory map of the relationship between nav_menu_item and block.attributes which is a significant cognative load.

I see both as ripe for the introduction of regressions as evidenced by the existing drift in the two codebases for similar functional requirements.

Why do we need this now?

It's worth asking whether it would be best simply to bring both implementations into sync and then make more effort to maintain the duplication by hand.

However, I would argue that the requirements and complexity of both features have already become significant and the overhead to maintaining these "by hand" is close to becoming (if not already) overwhelming.

As new requirements are placed upon both block and editor this situation will inevitably become even worse.

Aside: placeholder should remain in the Block

We are now looking to introduce a specific placeholder state for the Nav editor. This is going to introduce yet more complexity and was the initial inpetus for my proposal to introduce the @wordpress/menus package.

Some have argued (with some validity I might add) that the placeholder state should remain in the block and not be part of the editor.

Whilst this is indeed a valid point, and one I used to buttress my original proposal, I now see it as tangential to the requirement to introduce a shared @wordpress/menus package in order to reduce complexity in the Nav editor and block.

The complexity is already significant enough to merit finding a way to share code more effectively.

What is your proposed solution?

As you're aware by now, I'm proposing that we create a @wordpress/menus package (naming of this is up for debate - I'm not fixed on menus) where we can colocate code that is shared between block and editor. This will reduce maintenance overhead, reduce cognitive load on developers and improve the resilience of the two features.

It will also provide the additional benefit of allowing us to more easily isolate and test some of the (duplicate) code which is not currently covered by automated tests.

Existing precedent and whether to create co-dependent packages

I would draw your attention to the @wordpress/widgets package which was created to share code between Widgets and Customizer screens.

Whilst I'm aware that not everyone is in favour of such packages, it seems that the requirements of Gutenberg's development are necessitating some means of sharing code between packages. The discussion about how we do this feels like an architectural decision that is rapidly becoming something that needs to be addressed.

In the interests of moving forward in this particular instance however, I would argue that the existing precedent of the @wordpress/widgets package justifies the creation of a similar package for Navigation.

I appreciate that @mtias and @youknowriad may feel that in order to avoid compounding a pattern they are not in favour of we first need to solve the wider problem of code sharing reuse in packages. I would caution, however, that whilst we await the development of such a solution, the Navigation components will inevitably be impacted by the friction (as illustrated above) generated by excessive code duplication between the Navigation block and editor.

Conclusion

In conclusion, for the reasons outlined above, I recommend we proceed with the introduction of a shared @wordpress/menus package. This will:

• reduce complexity.
• reduce cognitive load on contributors and maintainers.
• reduce the likelihood of regressions across block and editor.
• reduce the likelihood of inconsistencies between block and editor.
• improve our test coverage for key logic and functionality.

I would draw your attention to the existing discussion on the PR where many of the points illustrated above were first introduced (particularly this one by @gwwar).

I look forward to us all producing a more consistent and reliable experience across Nav block and editor.

cc @gwwar @jasmussen @talldan @draganescu @Mamaduka

[getdave]

FYI I'll spin up a POC of this for evaluation.

[Mamaduka]

I like the idea and see the need for the common functionality package.

[jasmussen]

I'll defer to you on most of the pieces. Since I'm mostly focused on the CSS at the moment, I wanted to note that there's a great deal of incoming CSS with the navigation 🍔 menu (#30047), and after that, a separate overlay color set (#31149). Once these two land, in turn, I expect I will have to do a bit of CSS refactoring. And finally, there will be continued polish of the nav block setup state, as aspects of that continue to confuse in tests.

Those are the last big pieces, but they are can hopefully land in the next two weeks. But they are still big pieces that need to land. So I suppose a related question to that is: how much will this change affect the nav block CSS, if at all?

[getdave]

So I suppose a related question to that is: how much will this change affect the nav block CSS, if at all?

I don't anticipate the creation of this package affecting the CSS at all. Mostly it is moving shared logic and utilities to a common location to avoid duplication. I won't be touching the CSS.

If in the future we find the need to share CSS (I'd be surprised if it would be worth the effort or even possible) we can revisit the approach. I don't imagine that will happen prior to 5.8 though.

I hope that helps?

The wider question is if the Nav block is slated for WP 5.8 and Nav editor is not, then is it ok to create this package at this time? I'd like to defer to @talldan or @draganescu on this one as they've got more experience in this area. I presume that as Gutenberg as a whole will end up in 5.8 (whether or not the Nav Editor is enabled) it should be fine.

[pbking]

Would something like #30852 be able to take advantage of this?

[mtias]

I'm not super fond of introducing and publishing packages just to share code between a couple screens if it doesn't have inherent value. Could you go further into what specific things are intended to be included? The placeholder state should really be part of the block, for example.

[getdave]

@mtias @youknowriad @gwwar @Mamaduka @talldan @draganescu I've revised the description of this Issue to better illustrate why I believe it is necessary to introduce a shared @wordpress/menus package.

The specifics of what this package will contain is still something that needs work, but I don't want to proceed too far along with this level of detail unless there is broad agreement that there is merit in the proposal. Nonetheless, I have outlined the current areas of duplication and so it is possible to extrapolate from this the type of functionality that I would like to see in the new package.

I strongly believe that this package is a necessity and not a convenience. I believe we will see the benefits in allowing the Navigation components to evolve more rapidly and will fewer regressions.

Please do let me know if I'm good to go with developing the package further. FYI I see the first steps as normalizing the existing duplicating code in situ in individual separate PRs and then extracting to the PR introduces the package. This should make it easier to introduce the package if we decide to do so.

I look forward to your feedback 🙇‍♂️

[talldan]

I hadn't really thought of the implication of this being exposed as wp.menus—this code seems like internal implementation detail that would be best shared to reduce maintenance rather than something to expose so easily as a public API, but there's no grey area in the way the code is organised.

I definitely see the value of having this code shared in a separate package though, despite the fact that doing so makes it a public API.

Analysing why it's being proposed:

• The duplicated code had already diverged and caused some bugs.
• The block-library package where the block is implemented can't have a dependency on edit-navigation.

There aren't too many alternatives keeping it duplicated and writing some bespoke tests that check the duplicated functions have the same outcome. The other option available is to move it into another existing shared package, at which point it's still a public API, although it could be made experimental/unstable.

[youknowriad]

We can have packages that don't translate to wp.* globals like "icons" and "interface" today. The setup to make this happen though is not straightforward, it requires some changes to webpack configs for both core and Gutenberg.

[Mamaduka]

I think @getdave makes a good point.

Having menu items to block attributions mappers out of sync will easily lead to more bugs.

P.S. I've worked with menu items data model before (migrations, custom menu items), and I won't be able to remember all of it now if I had to save my life 😅

[gwwar]

Would it be worth revisiting this idea from @noisysocks in #29993 (comment)

(Added a screenshot to save folks a click):

We definitely have a need to share the code. I also think we'll run into this same problem whenever there's a shared code implementation for each WP editor. One question I have is do we fix the problem more generally, or opt for one-off packages as we need to?

[draganescu]

The specifics of what this package will contain is still something that needs work, but I don't want to proceed too far along with this level of detail unless there is broad agreement that there is merit in the proposal.

Feels to me from reading this that the key is precisely in the specifics:

1. what will the package contain?
2. will it be exclusively useful to DRY code, or is it otherwise interesting architecturally?

The idea to create packages in general in Gutenberg is summed up very well by @youknowriad: the packages should hold value on their own and be useable stand alone. The existing packages that do not comply to this idea should not be considered a pattern to be followed but some form of compromise to avoid.

I guess, the main question is what will the package contain? If the contents of the package are architecturally bound to be duplicated, and we can't find a way to add them to the block OR to the editor, then the compromise is technically justified. If the contents of the package are exposed in a way that they make sense on their own and can be useful when used directly, then the existence of the package is justified technically.

So far, seems to me, we're too keen on proving that code duplication is problematic. This is 100% true. But the usual ways of avoiding code duplication does not create a public API to support forever. If our tooling and tech allows us to remove duplication and to avoid public packages that provide little value then DRY is the way, otherwise we need to make sure there is no other better way to dedupe (e.g. @talldan 's idea to encapsulate the menu items to navigation block link blocks mapping in the navigation block is an example of a better way, although I don't know how possible it is).

So what will the package contain :) ?

[getdave]

Closing this for now as we've had less feature requirements driving this. We might need in future at which point it will be easier to answer the questions above.