Compatibility and Consistency issues in wp.hooks

[elliotcondon]

Issue Overview

The new wp.hooks logic introduced is not consistent with the existing PHP functions.
The parameters used in wp.hooks.addAction do not match the parameters used in add_action().

Example

The PHP add_action() function looks like this:
add_action( $tag, $callback, $priority, $accepted_args )

While the wp.hooks.addAction() function looks like this:
addAction( tag, namespace, callback, priority )

The difference here is that the JS version requires a 'namespace' as the 2nd parameter instead of allowing a namespace within the tag itself (as WP has done for years).

The Problem

The problem here is that many developers (including myself) have been using existing JS libraries such as https://github.com/carldanley/WP-JS-Hooks to implement wp.hooks.addAction() functionality for years. This library does follow consistency with WP PHP functions.

By changing the parameters, this will break any plugin already using a wp.hooks solution. That is 1 million+ ACF users (free and PRO).

The Solution

ACF aside, It makes sense to keep the new JS functionality consistent with its PHP counterpart.
The add_action() and add_filter() functions are responsible (in my opinion) for WP's success in becoming every developer's favorite CMS.

Please consider changing the the wp.hooks logic to reflect WP PHP core. This is in WP's best interest for compatibility, usability and consistency.

Thanks
Elliot

[gziolo]

Sharing here WP-JS-Hooks API for reference in the further discussion:

wp.hooks.addAction( 'tag', callback, priority );
wp.hooks.addFilter( 'tag', callback, priority );
wp.hooks.removeAction( 'tag', callback );
wp.hooks.removeFilter( 'tag', callback );
wp.hooks.doAction( 'tag', arg1, arg2, arg3 ); // any number of args can be included
wp.hooks.applyFilters( 'tag', content );

The difference here is that the JS version requires a 'namespace' as the 2nd parameter instead of allowing a namespace within the tag itself (as WP has done for years).

It has been discussed several times in the past both on GitHub (e.g. #5474) and on WordPress.org Slack during Core JS chats. It seems like we will discuss it again today during weekly Core JS chat in 5.5 hours :)

The biggest issue for the WordPress core with the implementation of WP-JS-Hooks is the fact that when you pass a callback when registering the hook you need to expose the reference to this callback globally to be able to remove it from plugins. This was the main reasons this 2nd namespace param was introduced in the first place. I don't think this is a big issue for the hooks maintained by plugins though.

I'm wondering if we take advantage of the nature of JS language and propose the following workaround to satisfy all parties involved:

const withCopyContentMenuItem = () => // something in here ...;
withCopyContentMenuItem.namespace = 'core/edit-post/more-menu/with-copy-content-menu-item';
addFilter(
	'editPost.MoreMenu.tools',
	withCopyContentMenuItem
);

In fact, in Gutenberg in the majority of cases, we declare callbacks before we add hooks so it wouldn't be much more work to apply namespace this way. However, my favorite solution would be to magically apply this namespace behind the scenes using Function.name as described in #5474 (comment). This might require some changes in a way how uglify is used with both Gutenberg and WordPress core.

[elliotcondon]

Hi @gziolo

Thanks for the info. Can you please elaborate on the following:

The biggest issue for the WordPress core with the implementation of WP-JS-Hooks is the fact that when you pass a callback when registering the hook you need to expose the reference to this callback globally to be able to remove it from plugins.

Are you talking about potential difficulties when trying to remove actions/filters?

If so, a good question to ask is: does this issue already exist in the PHP side of things?

• If not, how does PHP solve it?

[gziolo]

Are you talking about potential difficulties when trying to remove actions/filters?

Yes.

If so, a good question to ask is: does this issue already exist in the PHP side of things?

It is solved in PHP by allowing to provide string as a callback:

function gutenberg_meta_box_save_redirect( $location, $post_id ) { ... }

add_filter( 'redirect_post_location', 'gutenberg_meta_box_save_redirect', 10, 2 );

It is possible because this function name is accessible in the global scope. In JavaScript we use modules so those callbacks aren't exposed globally.

[elliotcondon]

Hi @gziolo

Thanks for clarifying, yes, this all makes sense now.
Here's a few ideas which I hope can help:

1. What happens in PHP if you pass in an anonymous function, rather than a 'string'

• does this create the same issue?

2. Perhaps one solution (similar to PHP) would be to provide the callback as an array of "context" and "callback"

3. Use a namespace within the "tag" similar to how jQuery allows this on events.
   https://api.jquery.com/event.namespace/

Thanks for looking into this!

[gziolo]

I'm sure

What happens in PHP if you pass in an anonymous function, rather than a 'string'

Yes, the same issue. WordPress core uses PHP 5.2 compatible code so it isn't the case here though because there are no closures :)

Some good ideas, similar to what I shared.

[adamsilverstein]

@elliotcondon Thanks for opening this issue for wider discussion.

I wanted to mention the wp.hooks is based directly on Carl Danley's hooks library you linked to. The namespace parameter was added after extensive discussion on the trac ticket: https://core.trac.wordpress.org/ticket/21170 - I suggest reading this ticket if you haven't already as it goes thru the reasoning for why the namespace parameter was added.

[adamsilverstein]

@elliotcondon

We explored removing namespace requirement in a few prs - WordPress/packages#106, WordPress/packages#108, WordPress/packages#113, still no final decision. The big advantage of having the namespace parameter is the ability to remove (and check for) the presence of a specific callback, which seems worth the extra effort to add the parameter.

If we land with the namespace as a required parameter, you should be able to write a shim that enables you to continue to support usage without a namespace parameter by intercepting the add_ calls. replace the original function, check the parameters and create a namespace automatically if possible.

[elliotcondon]

Hi @adamsilverstein

I think you are making a big mistake by adding in this "namespace" parameter. I can appreciate the functionality it brings, but the cost is too high.

There are millions of WP websites with ACF installed, all of which expect the wp.hooks.addAction() function to follow the same convention as the existing WP PHP core.

I'll get to work on an update for ACF which will avoid this issue, but please consider the cost of this decision. For all the ACF users who do not update in time, this will break their admin editing completely.

[adamsilverstein]

@elliotcondon we are still exploring the namespace parameter. can you give me a link to where your code makes use of wp.hooks? i want to make sure I understand how you use it. I read the docs, but couldn't find the matching code.

Are you using Carl Danley's implementation? is this the correct documentation for your JS hooks? http://www.advancedcustomfields.com/resources/adding-custom-javascript-fields/ - I don't see any use of wp.hooks.addAction in your docs - is that mapped from acf.add_action?

For all the ACF users who do not update in time, this will break their admin editing completely.

It is likely that wp.hooks will only be loaded by default in Gutenberg, so unless another plugin or theme enqueues it in the classic editor your functionality should remain intact.

[elliotcondon]

@adamsilverstein glad to hear you are still exploring options.

Yes, we are using WP-JS-Hooks.

We use a wrapper function called acf.add_action() which calls wp.hooks.addAction().
The source code for this function is found in our JS here: https://github.com/AdvancedCustomFields/acf/blob/master/assets/_build/js/acf.js#L151

Can you please explain your last point? You seem to be insinuating that ACF users won't be editing content via the new block editor...

When will you have a definitive answer about this issue? I will need to roll out an update ASAP for my current users.

[adamsilverstein]

@elliotcondon thanks for the code links, that helps me understand how you are using Carl's hooks library. It's unfortunate that the plugin also uses the wp namespace. So I am assuming that when you load up a page with hooks running, they fail because of the required namespace parameter?

Can you provide some sample code that uses a hook? Also, can you please test the code on WordPress/packages#113 to verify that fixes the issue for you?

[danielbachhuber]

Thanks for your time and attention on this issue, @elliotcondon. At this point so close to release, we're unlikely to remove the namespace argument from registration. Feel free to weigh in if you have additional information to share.

[elliotcondon]

Hi @danielbachhuber

Thanks for the update. That's a shame. Consistency is key in helping developers embrace and promote new functionality.