WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 15 hours ago

#21170 assigned feature request

JavaScript actions and filters

Reported by: koopersmith Owned by: adamsilverstein
Milestone: 5.0 Priority: normal
Severity: normal Version: 3.4
Component: General Keywords:
Focuses: javascript Cc:

Description

The concept of adding JavaScript actions and filters has been tossed around for some time. We've experimented with various configurations of actions in both the fullscreen and customizer APIs, and they've proven their utility enough to graduate them to a core feature in their own right.


I think that a good events API should satisfy these parameters:

  1. Support jQuery-style dot namespacing to allow functions to be easily removed.
  1. Should (likely) support priorities. While seemingly random numbers aren't fun to use, it allows plugins to cooperate without having to know of each other's existence. We can't expect plugin authors to rearrange the array of callbacks.
  1. Should not force functions to have unique IDs. Anonymous functions are extremely common in JavaScript — forcing them to be named is contrary to the nature of the language.
  1. Should be structured as a mixin. The global event loop should be an instance of the core Events object. Using a mixin will allow developers to easily create event loops for their own plugins (to prevent polluting the global namespace — think about large plugins, like bbPress). An events mixin will also enable developers to create more powerful abstractions, such as observable values, collections, and pretty much any structural JS object you can dream up.
  1. Should allow the looping process to be overwritten. This will result in less code and added flexibility. The only difference between actions and filters is how they handle the callbacks object. There are other types of looping processes that could be beneficial in JS. One example would be returning false if any callback returns false, which could be used to stop a process, much like the native event.stopPropagation method.

Why not use custom jQuery events?

Custom jQuery events are great when we need to trigger actions on a DOM element. Triggering plain events on the body element (or any other hidden element) is not performant — every jQuery event normalizes an DOM Event object, which we then completely ignore.

Should we require jQuery to use the API?

I'm not sure. jQuery.Callbacks may be a helpful solution here, provided we can properly integrate priorities and namespacing. jQuery.Callbacks also only requires jQuery.each and jQuery.extend, so writing a shim that doesn't use the rest of jQuery would not be exceptionally difficult. Either way, switching between one and the other as we develop should not be exceptionally difficult.

Attachments (21)

21170.patch (6.2 KB) - added by azaozz 5 years ago.
wp-hooks.js (2.5 KB) - added by mikeschinkel 5 years ago.
wp-hook.js which is a copy of the same functionality from Sunrise.
21170-1.patch (20.5 KB) - added by TV productions 4 years ago.
Implemented the wp hooks from https://github.com/gcorne/WP-JS-Hooks/
21170-2.patch (22.3 KB) - added by TV productions 4 years ago.
21170.diff (8.1 KB) - added by adamsilverstein 2 years ago.
21170.2.diff (8.6 KB) - added by adamsilverstein 12 months ago.
21170.3.diff (8.6 KB) - added by adamsilverstein 12 months ago.
21170.4.diff (15.6 KB) - added by adamsilverstein 12 months ago.
21170.5.diff (1.6 KB) - added by aduth 9 months ago.
My take on @adamsilverstein's patch with consideration of previous remarks
21170.5.2.diff (11.8 KB) - added by aduth 9 months ago.
(Fixed 21170.5.diff)
21170.6.diff (16.1 KB) - added by adamsilverstein 8 months ago.
21170.7.diff (16.0 KB) - added by adamsilverstein 8 months ago.
fixes for jshint/travis
21170.8.diff (17.4 KB) - added by adamsilverstein 5 months ago.
21170.9.diff (18.3 KB) - added by adamsilverstein 5 months ago.
21170.10.diff (19.1 KB) - added by adamsilverstein 4 months ago.
21170.11.diff (19.2 KB) - added by adamsilverstein 4 months ago.
21170.12.diff (26.7 KB) - added by adamsilverstein 4 months ago.
21170.13.diff (31.2 KB) - added by adamsilverstein 3 months ago.
21170.14.diff (32.3 KB) - added by adamsilverstein 3 months ago.
media-button-filter-example.diff (3.4 KB) - added by adamsilverstein 2 months ago.
21170.15.diff (32.3 KB) - added by adamsilverstein 7 weeks ago.
revert commit

Download all attachments as: .zip

Change History (217)

#1 @lgedeon
5 years ago

  • Cc luke.gedeon@… added

#2 @WraithKenny
5 years ago

  • Cc Ken@… added

#3 @scribu
5 years ago

I think 1) is just a consequence of 3) and 3) follows naturally, since in JavaScript you have to pass functions around directly, instead of strings.

I'm not sure about 5). Why would we need this in JS, but not in PHP?

#4 @sabreuse
5 years ago

  • Cc sabreuse@… added

@azaozz
5 years ago

#5 @vhauri
5 years ago

  • Cc vhauri added

#6 @azaozz
5 years ago

21170.patch is a simple "pub/sub" based on Amplify.js. It implements the above points and can be used as example/base for this.

#7 follow-up: @vhauri
5 years ago

+1 for staying away from the DOM to do this.

I feel like the Callbacks object would be a preferable solution to using a modified version of something like AmplifyJS, as it allows us to develop this with only jQuery as a dependency (and even there only the two methods .each() and .extend() ), rather than relying on or attempting to duplicate the functionality of a 3rd-party plugin, which will be getting updated of its own accord, etc.

#8 in reply to: ↑ 7 @azaozz
5 years ago

Replying to vhauri:

... as it allows us to develop this with only jQuery as a dependency...

The above script doesn't have any dependencies. That may be preferable for some cases (front-end use?). The corresponding jQuery methods (each(), extend(), etc.) are tuned for wider range of uses like traversing and extending complex objects or the DOM which is not needed at the moment. So using a for... in loop instead of jQuery.each() and obj[key] = val; instead of jQuery.extend() is faster and keeps the script self-contained.

Last edited 5 years ago by azaozz (previous) (diff)

#9 @vhauri
5 years ago

@azaozz The above script is a maintained plugin that is being constantly changed (i.e. it is itself the dependency). I'm not sure how we'd maintain cross-compatibility with it, but if your thought is that we use it as a basis for a vanilla JS library (one that doesn't maintain any sort of compatibility with the latest and greatest version of AmplifyJS) that does the actions/hooks stuff, that sounds reasonable. My preference for basing off jQuery is that the Callbacks object will likely be maintained for us, so code that utilizes it will also be somewhat future-proof. I'm just not sure whether that'll be the case for AmplifyJS, but obviously if we're just using the codebase as it stands now as a branching point, no issues.

I think this speaks to the larger issue of whether jQuery itself should be assumed. Your point about the front-end (i.e. in comment-reply.js) does make me think that maybe there's a need to separate this from jQuery, although I do feel that jQuery is the de-facto JS library that WP uses at this point.

#10 @usermrpapa
5 years ago

  • Cc steve@… added

#11 in reply to: ↑ description @mikeschinkel
5 years ago

  • Cc mikeschinkel@… added

Replying to koopersmith:

The concept of adding JavaScript actions and filters has been tossed around for some time. We've experimented with various configurations of actions in both the fullscreen and customizer APIs, and they've proven their utility enough to graduate them to a core feature in their own right.

This is an excellent ticket, really glad to see it. We've been using something very much like this in our client's systems for the past year. I've copied the code from plugin below changing the name from our plugin's name to 'wp'; I've also added as an attachment.

Basically what we did was added a "hooks" object as a wp property of the jQuery object, i.e. $.wp:

/*
  jQuery Hooks for WordPress

  Examples:

  // Add three different test actions
  $.wp.addAction( 'test', function() { alert('Foo!'); } );
  $.wp.addAction( 'test', function() { alert('Bar!'); } );
  $.wp.addAction( 'test', function() { alert('Baz!'); } );

  // Remove the first one
  $.wp.removeAction( 'test', 'test_1' );

  // Do the remaining test actions
  $.wp.doAction( 'test' );


  // Add a filter somewhere
  $.wp.addFilter('filterOptions',function(options) {
    // Do stuff here to modify variable options
    return options;
  } );

  // Use the filter here
  options = $.wp.applyFilters('filterOptions',options);

 */
jQuery(document).ready(function($) {
  $.wp = {
    /**
     * Implement a WordPress-link Hook System for Javascript 
     * TODO: Change 'tag' to 'args', allow number (priority), string (tag), object (priority+tag)
     */
    hooks: { action: {}, filter: {} },
    addAction: function( action, callable, tag ) {
      jQuery.wp.addHook( 'action', action, callable, tag );
    },
    addFilter: function( action, callable, tag ) {
      jQuery.wp.addHook( 'filter', action, callable, tag );
    },
    doAction: function( action, args ) {
      jQuery.wp.doHook( 'action', action, null, args );
    },
    applyFilters: function( action, value, args ) {
      return jQuery.wp.doHook( 'filter', action, value, args );
    },
    removeAction: function( action, tag ) {
      jQuery.wp.removeHook( 'action', action, tag );
    },
    removeFilter: function( action, tag ) {
      jQuery.wp.removeHook( 'filter', action, tag );
    },
    addHook: function( hookType, action, callable, tag ) {
      if ( undefined == jQuery.wp.hooks[hookType][action] ) {
        jQuery.wp.hooks[hookType][action] = [];
      }
      var hooks = jQuery.wp.hooks[hookType][action];
      if ( undefined == tag ) {
        tag = action + '_' + hooks.length;
      }
      jQuery.wp.hooks[hookType][action].push( { tag:tag, callable:callable } );
    },
    doHook: function( hookType, action, value, args ) {
      if ( undefined != jQuery.wp.hooks[hookType][action] ) {
        var hooks = jQuery.wp.hooks[hookType][action];
        for( var i=0; i<hooks.length; i++) {
          if ( 'action'==hookType ) {
            hooks[i].callable(args);
          } else {
            value = hooks[i].callable(value, args);
          }
        }
      }
      if ( 'filter'==hookType ) {
        return value;
      }
    },
    removeHook: function( hookType, action, tag ) {
      if ( undefined != jQuery.wp.hooks[hookType][action] ) {
        var hooks = jQuery.wp.hooks[hookType][action];
        for( var i=hooks.length-1; i>=0; i--) {
          if (undefined==tag||tag==hooks[i].tag)
            hooks.splice(i,1);
          }
        }
      }
  }
});

This code has worked really well for us and stood the test of time, though I do have a TODO comment in the code to enable priorities. Maybe it could be a base for something in 3.5?

@mikeschinkel
5 years ago

wp-hook.js which is a copy of the same functionality from Sunrise.

#12 follow-up: @azaozz
5 years ago

Replying to vhauri:

...but if your thought is that we use it as a basis for a vanilla JS library (one that doesn't maintain any sort of compatibility with the latest and greatest version of AmplifyJS) that does the actions/hooks stuff, that sounds reasonable.

Exactly, the patch is an earlier attempt to implement this as simple as possible. It was based off of AmplifyJS (it "borrows" some code from there). However it is quite different from the original and doesn't interfere with it, and there would be no need for any maintenance when AmplifyJS is updated.

The jQuery Callbacks() looks like a good candidate but is missing some key features like priorities and namespaces. Considering it would be preferable to keep this API self-contained, perhaps we can settle for a simpler solution using few loops instead of trying to patch/extend Callbacks(). That would need less maintenance too.

#13 in reply to: ↑ 12 ; follow-up: @vhauri
5 years ago

Replying to azaozz:

The jQuery Callbacks() looks like a good candidate but is missing some key features like priorities and namespaces. Considering it would be preferable to keep this API self-contained, perhaps we can settle for a simpler solution using few loops instead of trying to patch/extend Callbacks(). That would need less maintenance too.

Callbacks does offer the .fired() method to test if a particular callback has been fired: http://api.jquery.com/callbacks.fired/. Being able to tell specifically whether or not a particular callback or list of callbacks has fired would be useful. That doesn't solve the problem of actually being able to call one method before another, but I do think it's a worthwhile feature that should be added into whatever solution ends up being used.

#14 in reply to: ↑ 13 @azaozz
5 years ago

Replying to vhauri:

Callbacks does offer the .fired() method to test if a particular callback has been fired... I do think it's a worthwhile feature that should be added into whatever solution ends up being used.

Agree, there is similar functionality in PHP: did_action() and in the above example: wp.didAction(). It wouldn't be hard to extend this so it can be used for each callback, i.e. support both wp.didAction(action) and wp.didAction(action.namespace).

#15 @sirzooro
5 years ago

  • Cc sirzooro added

#16 @ocean90
5 years ago

  • Cc ocean90 added

#17 @CaptainN
5 years ago

Just thought I'd offer a somewhat different model (especially since you don't want to go with a standard DOM implementation).

The idea is modeled after Robert Penner's AS3Signals, which itself if modeled after C#'s event model. It has a couple of advantages compared with DOM style events (and some disadvantages - propagation would need to be implemented - that's not as much of an issue for WP style filters and actions though) - the primary benefit IMO is it uses properties instead of strings for event types (louder errors when you type something wrong).

Here is a tiny implementation of Signals in JS:
https://github.com/CaptainN/SignalsLite.js/blob/master/src/SignalLite.js

This library doesn't have the requirements listed in the ticket, but they could be added pretty easily (I'll probably add them this weekend, cause they seem like nice features to have).

Here's the primary difference in syntax for the end user:

// DOM style events:
/* event is of type Event */
wphooks.actions.addEventListener( "click.namespace", function( event ) {}, 2 /* priority */);
// or
function funcRef( event ) {}
wphooks.actions.addEventListener( "click.namespace", funcRef, 2  /* priority */);
wphooks.actions.removeEventListener( "click.namespace", funcRef );

// signals (with unimplemented priority and namespacing):
/* obj is of whatever type you want it to be - much more like WP filters */
wphooks.actions.clicked.add( "namespace", function( obj ) {}, 2 /* priority */ );

// or
function funcRef( obj ) {}
wphooks.actions.clicked.add( funcRef, 2 /* priority */ );

// removes all namespaced listeners by string
wphooks.actions.clicked.remove( "namespace" );

// removes only the one listener by reference (DOM style)
wphooks.actions.clicked.remove( funcRef );

You'd construct it like so:

window.wphooks = {
    actions: {
        clicked: new SignalLite(),
        other_custom_action: new SignalLite()
    }
    filters: {
        some_filter: new SignalLite(),
        some_other_filter: new SignalLite()
    }
}

// add additional hooks elsewhere (you could do an addAction abstraction too).
wphooks.actions.my_added_action = new SignalLite();

dispatch works like this:

// maps "this" in the listener to thisObj
wphooks.actions.clicked.target = thisObj;
// obj is whatever you need it to be for this hook
wphooks.actions.clicked.dispatch( obj  );

This may be out of left field, but I thought I'd throw it in the discussion. :-)

#18 @scribu
5 years ago

In the signals model, you have to create a new SignalLite instance for each hook. That seems like a hindrance rather than an advantage.

#19 follow-up: @CaptainN
5 years ago

It's a lot of very small objects (as apposed to a lot of strings in other models), is that really a big deal?

You could also use a factory method of some kind to construct those (and to add/remove custom actions so plugin authors wouldn't have to instantiate any Signal instances directly).

I was also thinking of a way to do lazy instantiation using a getter/setter model (you still get the better error checking on property access, without instantiating all those objects up front) - but that IE monster keeps it from happening (there are ways in IE even if they are bit hacky).

#20 in reply to: ↑ 19 @azaozz
5 years ago

Replying to CaptainN:

It's a lot of very small objects (as apposed to a lot of strings in other models)

Yes, I can see some advantages in that. If each hook name refers to an instance, we could have couple of basic methods there, add(), remove(), fired(), etc. even if add() is just [].splice() and remove is [].slice().

The important thing is to keep the callbacks (which could be instances of a small "callback" class) in a true JS array so the priority is maintained. Unlike PHP, JS doesn't guarantee that ordering in objects will be maintained.

You could also use a factory method of some kind to construct those (and to add/remove custom actions so plugin authors wouldn't have to instantiate any Signal instances directly).

Yeah, that's a must. Another somewhat similar implementation of custom events is in TinyMCE. It uses instances for both events and callbacks and seems pretty well build (doesn't have priority for the callbacks but has addToTop() method): http://www.tinymce.com/wiki.php/API3:class.tinymce.util.Dispatcher,
code at: https://github.com/tinymce/tinymce/blob/master/jscripts/tiny_mce/classes/util/Dispatcher.js

#21 @johnbillion
5 years ago

  • Cc johnbillion added

#22 @mikeschinkel
5 years ago

Is this now actually planned for v3.5?

#23 @CaptainN
5 years ago

  • Cc CaptainN added

SignalsLite.js now has most of these features. It's missing only the priority flag - namespaces, addToTop and safe dispatching are all implemented.

https://github.com/CaptainN/SignalsLite.js

There are even a bunch of unit tests (check there for usage for now, if you are curious):
http://www.unfocus.com/SignalsLite.js/tests/AllTests.html

In case there is any interest.

#24 follow-up: @CaptainN
5 years ago

Hmm, one thing I just noticed about SignalLite, and a lot of the other JavaScript callback systems, is they don't really provide a way to do WordPress style filters (other than having an additional whole set of callbacks). That is, send a value to the listener, and then deal with whatever the listener returns between each listener.

It seems a JavaScript way to do something like that would be the DOM Event model of passing an object with some properties that can be used to apply the filter, then the system could just ignore return values. But that's not very WordPressy.

Maybe the way to go is to add a return handler that the implementer is meant to support in the SignalLite constructor, if they choose:

var signaled = new SignalLite( null /* default target */,
    function( value ) {
        // do something with the returned value
    }
);

This would actually create a property on the signal:

// you could also use this alternative syntax
var signaled = new SignalLite();
signaled.eachReturned = function( value ) {
    // do something with the returned value
}

Here's what an implemented filter might look like:

/// in core/plugin code - implementation
var value_tofilter = "some value of stuff";

// use this or a factory method
window.wpFilter = {
  value_filter: new SignalLite( null,
    function( value ) {
        value_tofilter = value;
    }
  )
}

// ... in client code - user of the filter
wpFilter.value_filter.add( function( value ) {
    value.replace( "some", "THE" );
    return value;
} );

// ... back in core code - dispatching the filter
value_tofilter = wpFilter.a_filter.dispatch( value_tofilter );

console.log( value_tofilter );
// LOG: THE value of stuff

I'll probably go ahead and implement that. :-)

#25 in reply to: ↑ 24 @azaozz
5 years ago

Replying to CaptainN:

Hmm, one thing I just noticed about SignalLite, and a lot of the other JavaScript callback systems, is they don't really provide a way to do WordPress style filters...

The JS way to do something similar is to pass around simple objects. As they are always passed by reference changing a value in a callback function would change it "everywhere".

Was planning to add "filters" and handle that automatically in the above example but passing object(s) is doable with the "actions" too, so not sure if it's needed. The only reason is that it would make this API more familiar to developers that are used to the PHP API.

#26 @CaptainN
5 years ago

Yeah, the JS way is to pass by reference, for sure. I added support to SignalLite anyway, because it was a gap (you can even filter by value):

var signaled = new SignalLite();

var value_tofilter = "some value of stuff";

var wpFilter = {
	value_filter: new SignalLite( null,
		function( value, args ) {
			value_tofilter = value;
			// set the argument so it'll pass modified to the next listener
			args[ 0 ] = value;
		}
	)
};

wpFilter.value_filter.add( function( value ) {
	value = value.replace( "some", "THE" );
	return value;
} );

wpFilter.value_filter.add( function( value ) {
	value = value.replace( "stuff", "things" );
	return value;
} );

wpFilter.value_filter.dispatch( value_tofilter );

equal( value_tofilter, "THE value of things", '"some" should be "THE" and "stuff" should be "things"' );

#27 @Mamaduka
5 years ago

  • Cc georgemamadashvili@… added

#28 @carldanley
5 years ago

Hi all,

I've been working with lgedeon to compile a list of requirements for this library and then spent some time building something custom to support what we might need. Long story short, let me jump into it:

Supported Features:

  • jQuery style dot namespacing
    • Can add/remove namespaces like so:
      $wp.hooks.addAction( 'some.crazy.action', func, 5 );
      $wp.hooks.addAction( 'some.crazy', func2, 7);
      $wp.hooks.removeAction( 'some.crazy.action' );
  • Supports method chaining to create a jQuery-like function chain:
    $wp.hooks.addAction().addFilter().removeFilter().doAction();
  • Priorities
    • Ability to pass applicable callbacks through a process method of choice so filters can be processed in predefined functions. The object that holds the predefined filtering methods can also be worked to support extensions in case developers want to write their own filtering methods. In example:
      $wp.hooks.doAction( 'some.action', { 'some' : 'data' }, 'processingMethod' );
      *note: also works for applyFilter().
    • Currently has support for handling processing callbacks through a default 'sequential' method; I do have plans to implement the following methods:
      • minValue
      • maxValue
      • allTrue
      • allFalse
  • Supports unique identifiers
  • Anonymous functions are supported:
    $wp.hooks.addAction( 'anonymous.funcs', function(){ /* do something */ }, 30 );
  • Checks for a $wp global and binds itself to $wp.hooks
    • $wp was used in case we decide to create other library functions, etc.

Supported Functions:

  • addAction( action, callback, priority )
  • doAction( action, data, processMethod )
    • processMethod is currently string only but when I add support for registering new process methods, I intend on adding support for passing a defined or anonymous function directly through the processMethod parameter.
  • removeAction( action )
  • addFilter( filter, callback, priority )
  • applyFilter( filter, data, processMethod )
    • see note for 'processMethod' under the doAction() function.
  • removeFilter( filter )
  • didAction( action )
    • returns false or an integer count
  • didFilter( filter )
    • returns false or an integer count

If you have questions concerning logic and/or code implementation, please refer to my code commenting first. I have tried to heavily comment the API in areas that I thought may need further explanation or where I had ideas for future improvements. I will patch the github as necessary.

Also wanted to note that, when compressed, this library is only around 2kb ( so far ).

Code can be found on my github here: https://github.com/carldanley/WordPress-Action-Filter-Library
Demo can be temporarily found here: http://codebyter.dyndns.info/wp.hooks/

Last edited 5 years ago by carldanley (previous) (diff)

#29 follow-up: @azaozz
5 years ago

Looks very interesting. Couple of "first look" notes:

  • The complex namespace implementation looks good. However I'm not sure we need to go there. The term "namespace" is used loosely in the above example. It works about the same like in jQuery event namespacing and is more of a "unique identifier" than a real namespace. (That's my fault btw, should have called it "identifier"). It can be implemented as above: addAction('actionname.identifier', function(args){...}); or as separate arg: addAction('actionname', 'identifier', function(args){...});.

Generally the actions (and filters) are a way to call specific function at a specific time. Having a complex structure/tree of actions is not necessary as they are all called at that specific time anyway (same as attaching events to the DOM).

Perhaps if we are implementing "stop execution if any callback returns false" (and that's a big if, since there's no such functionality in the PHP version), having a more complex structure of identifiers (namespaces) would allow for one branch to be stopped while other branches continue. Still, that would be a "border case" functionality. (Normally a plugin would register one callback with addAction and would expect it to be executed every time when doAction is triggered. For a "tree" structure to work, plugins would have to share the "branch names" which is quite hard to do, or we will end up with a big tree with many branches that have only one "leaf" (callback) each, and that's not useful).

  • Not sure about support for 'processingMethod'. Imho it's better/more important to be able to directly set the scope for the callbacks (all similar libraries support that). It would be good if you can add some usage examples for this.
  • Chaining might be useful in some cases, however not all methods can be "chainable" as applyFilters will have to return the filtered value. Maybe it would be better to leave chaining off and return true/false on whether addAction/addFilter/removeAction/removeFilter was successful or not.

#30 in reply to: ↑ 29 ; follow-up: @carldanley
5 years ago

It was fun to write! Couple of responses/questions:

The term "namespace" is used loosely in the above example.

I agree, it was hard coming up with a term that encapsulates the object for both actions & filters. If you looked through the demo, they are essentially treated the same, except for how they are processed; the only slight variance is that filters can modify the value passed to it. That said, what do you think we could call these "objects" to help reduce confusion and standardize it? I will make the changes in comments/code to reflect the more accurate choice of words.

Having a complex structure/tree of actions is not necessary as they are all called at that specific time anyway (same as attaching events to the DOM).

Are you referencing how the objects for filters & actions are bound with 3 properties ( id, callbacks & children ) and sort of nested within each other? What if we did the following:

  • rework the internal storage to have 2 variables or keys for 'actions' and 'filters'.
  • when addAction() is called with a namespace, split it and find every hierarchical identifier to check to see if they exist, creating them, if they don't already exist, as you go, ie. you add "my.awesome.func", build an array of "levels" of identifiers ( [ 'my', 'my.awesome', 'my.awesome.func' ] ) and then loop through the array. The identifiers could be hashed or literally written as the exact key with string filter to make sure there are no illegal characters as a key within the corresponding array.

Chaining might be useful in some cases, however not all methods can be "chainable" as applyFilters will have to return the filtered value.

Just to point out that not all methods in jQuery are 'chainable' either, example; $( object ).height(); or $( object ).width(); I think for this particular case chaining makes good sense when doing something. In terms of useability, I'd rather see/type

$wp.hooks.addAction().addAction().addFilter().addFilter();

than

$wp.hooks.addAction();
$wp.hooks.addAction();
$wp.hooks.addFilter();
$wp.hooks.addFilter();

It's easier to type the first example than the latter.

What do you think? I appreciate the feedback and am willing to rework logic on this. I've been pumped ever since lgedeon mentioned this ticket to me.

Last edited 5 years ago by carldanley (previous) (diff)

#31 in reply to: ↑ 30 ; follow-up: @azaozz
5 years ago

Replying to carldanley:

... they (actions and filters) are essentially treated the same, except for how they are processed; the only slight variance is that filters can modify the value passed to them.

Right, they are handled the same way in PHP too. Essentially they are the same thing except for filters the returned value of each callback is passed on to the next callback as first arg, and returned at the end.

Are you referencing how the objects for filters & actions are bound with 3 properties ( id, callbacks & children ) and sort of nested within each other?

Uh, sry I didn't explain that well. Was referring to support for more than one namespace/identifier, like: actionname.namespace.other.third. When parsed that would create a tree-like structure for each action that doesn't seem needed as all actions would be called regardless of the structure. So a "flat" structure, i.e. one branch (actionname) with many leaves (callbacks) would work just as well. The .namespace/identifier bit would only be used to identify the callback, so each "leaf" on the "branch" would have a name and we would be able to easily remove anonymous "leaves" by branch name + leaf name :)

(In 21170.patch the term "namespace" should be read "identifier" as that probably describes it's role better).

Also thinking the namespaces/identifiers should be required as that would make removing anonymous functions easy. As far as I see the most common use of this would be with anonymous functions: `addAction('init.my-thing', function(){...});

What if we did the following:

  • rework the internal storage to have 2 variables or keys for 'actions' and 'filters'.

Actions and filters should be kept separate, i.e. addAction('init') makes this an "action node", there can't be a filter 'init', applyFilters('something', val) would be another node, etc. If we create the nodes as instances of a small class, or even as simple objects, we can store the nodeType in each.

  • when addAction() is called with a namespace, split it and find every hierarchical identifier to check to see if they exist, creating them, if they don't already exist, as you go, ie. you add "my.awesome.func", build an array of "levels" of identifiers ( [ 'my', 'my.awesome', 'my.awesome.func' ] ) and then loop through the array.

Not sure there's a need for that hierarchy (as explained above). Perhaps some usage examples would help here.

...I think for this particular case chaining makes good sense when doing something. In terms of useability, I'd rather see/type

$wp.hooks.addAction().addAction().addFilter().addFilter();

Yeah, chaining is definitely handy. The question is whether it's better to support chaining or to return true/false on success/failure when adding or removing actions and filter. The example in 21170.patch (and jQuery.Callbacks() I believe) would only add a particular callback once, so returning false on attempts to add it again makes sense. I'm 50/50 on this at the moment, would be good to get more opinions :)

What do you think? I appreciate the feedback and am willing to rework logic on this. I've been pumped ever since lgedeon mentioned this ticket to me.

Yes, it's an interesting challenge. Perhaps if we get it right, it can also be released as a stand-alone JS class.

#32 in reply to: ↑ 31 ; follow-up: @carldanley
5 years ago

Replying to azaozz:

The .namespace/identifier bit would only be used to identify the callback, so each "leaf" on the "branch" would have a name and we would be able to easily remove anonymous "leaves" by branch name + leaf name

So, as an example, when we use the action identifier 'actionname.namespace.other.third', I was thinking that there were several identifiers for this particular action. The action being 'actionname' and then several tiers of identifiers: 'actionname.namespace', 'actionname.namespace.other', 'actionname.namespace.other.third'. Each unique identifier tier would simply be it's own key in a flat array. Essentially, the callback passed lives in the 'actionname.namespace.other.third' array key. However, the problem I see with this is as follows:

addAction( 'foo.bar', callback1 );
//Actions:
//ACTIONS[ 'foo.bar' ] = [ callback1 ];

addAction( 'foo.bar.test', callback2 );
//Actions:
//ACTIONS[ 'foo.bar' ] = [ callback1 ];
//ACTIONS[ 'foo.bar.test' ] = [ callback2 ];

addAction( 'foo.bar.test.foo', callback3 );
//Actions:
//ACTIONS[ 'foo.bar' ] = [ callback1 ];
//ACTIONS[ 'foo.bar.test' ] = [ callback2 ];
//ACTIONS[ 'foo.bar.test.foo' ] = [ callback3 ];

So when we doAction( 'foo.bar' ), we'd have to loop through each key in the flat array finding keys prefixed with 'foo.bar' and execute them. To me, this could get very heavy ( especially since we'd have to use for( var k in arr ) ) in different areas of a site with a lot of plugins installed. To combat this, I created the tree for easier access in calling the 'children' identifiers. Meaning, if we called 'foo.bar' in this example, we would know that 'foo.bar' would have an array of callbacks that needed execution and to find the children, we know the object had a property called children where 'foo.bar.test' and 'foo.bar.test.foo' lived in the same manner that their ancestor, 'foo.bar', was created ( as an object with properties like 'callbacks' and 'children' ). I really like the flat array approach, I was just hesitant because of look-up speed for doing anything. You could also make the value of each unique array key an object with 2 properties ('callbacks' and 'siblings' where 'siblings' would store an array of values that could be used to find siblings when looking up for deletion/execution)

If we create the nodes as instances of a small class, or even as simple objects, we can store the nodeType in each.

Consider the following:

var appendHookToArray = function( arr, hookIdentifier, data ){
	var identifiers = hookIdentifier.split( '.' );
	var levels = [], identifier;
	
	for( var i = 1, len = identifiers.length; i <= len; i++ ){
		identifier = identifiers.slice( 0, i ).join( '.' );
		if( arr[ identifier ] === undefined ){
			arr[ identifier ] = [];
			if( i === len ){
				//just push it since we know it's a new identifier
				arr[ identifier ].push( data );
			}
		}
		else if( i === len ){
			//insert the callback now
			arr[ identifier ] = insertHookByPriority( arr[ identifier ], data );
		}
	}
	
	return arr;
};

So then, when addAction is called all it does is this:

SELF.addAction = function( action, callback, priority ){
	if( typeof callback !== 'function' ){
		return SELF;
	}

	ACTIONS = appendHookToArray( ACTIONS, action, {
		'callback' : callback,
		'priority' : ( priority || 10 )
	} );
	return SELF;
};

With 'ACTIONS' being a private class property ( 'FILTERS' would also exist as something separately. ) ACTIONS/FILTERS will def. need to be kept separate but I think that adding a 'nodeType' property to the object created is a little bit of an unnecessary waste, if i'm understanding correctly, meaning: I don't think it's necessary to specify nodeType of 'action' for 100+ different actions, as an example. We already know that it's an action because we have to access the 'ACTIONS' variable and, for this example, storing a 'nodeType' for each ACTION of value 'action' would mean making the array 600+ bytes heavier at run-time. I would use the same new 'object' class for both actions and filters. Thoughts?

Yeah, chaining is definitely handy. The question is whether it's better to support chaining or to return true/false on success/failure when adding or removing actions and filter.

You're also making me sway on chaining vs. true/false. It would interesting to hear other opinions as well.

Perhaps if we get it right, it can also be released as a stand-alone JS class.

That's what I'm shooting for! :)

Version 0, edited 5 years ago by carldanley (next)

#33 in reply to: ↑ 32 ; follow-up: @azaozz
5 years ago

Replying to carldanley:

I understand what you're saying now. The only question I have is how do we traverse the array to find simulated 'parents' and 'children'.

Yes, that's exactly the thing. This is modelled after the PHP actions API that has been in WP for years and works quite well. There aren't any sub-divisions / sub-branches / children and grandchildren in an action. That keeps things simple. Each action is a single branch that only has leaves (callbacks). It also matches the DOM events API, there's no 'click.one', click.other', etc. When 'click' is triggered, everything attached to it is fired.

If doAction( 'some.action' ) was called and we know there were several actions that existed such as 'some.action.a' and 'some.action.b', would we first find 'some.action' and then maybe look to a 'family' property that will indicate who the parent/children are? I don't think traversing each key in this flat array and determining who is prefixed with the correct action/identifier is the safest way, especially using for( var in ) to iterate. Thoughts?

If we stick to the current model,

doAction( 'some.action' )
doAction( 'some.action.a' )
doAction( 'some.other' )

will not be supported. The action for all these would be

doAction( 'some' )

and all callbacks that were registered with

addAction( 'some.thing', function(){...} )
addAction( 'some.action', function(){...} )
addAction( 'some.identifier', function(){...} )

would be fired. So in addAction() the identifier only applies to the callback and has nothing to do with the action itself.

On the other hand, as I said in comment:29, perhaps we could support "true" namespacing for the actions. That would open some advanced possibilities to additionally "filter" which callbacks are called.

In this case each action would be a container of many actions that start with the same name, and each child action can also be a container for even more actions (grandchildren), and so on. So an action would contain both callbacks and other actions. This would complicate the API quite a bit even if we limit the depth to, lets say 3 levels.

Perhaps we should consider all pros and cons. For example, lets say we have an action 'init' with several namespaces/children: 'init.some', 'init.some.more', 'init.other'.

  • Firing 'init' would fire all callbacks registered directly under 'init', plus all callbacks registered under 'init.some', plus all callbacks registered under 'init.some.more', plus all callbacks registered under 'init.other', plus... etc. Exactly the same thing can be achieved by firing several actions: 'init', 'init_some', 'init_some_more', 'init_other', etc. one after the other.

So with namespaced actions we only need:

doAction( 'init' );

without namespaces we need:

doAction( 'init' );
doAction( 'init_some' );
doAction( 'init_some_more' );
doAction( 'init_other' );

and we have control over which action is fired first, second, etc.

Advantage of namespacing: shorter code, disadvantage: no control over firing order.

  • Setting a namespace first then firing the action. Lets say we can set the namespace to '.some'. Firing 'init' should fire only callbacks under 'init.some' and 'init.some.more'. That's still exactly the same as firing 'init_some' and 'init_some_more' as separate actions.

With namespaced actions we need:

setNamespace( 'some' );
doAction( 'init' );
setNamespace( '' ); // depends on whether we reset the namespace after each action

without namespaces we need:

doAction( 'init_some' );
doAction( 'init_some_more' );

and we can choose to do 'init_some_more' before 'init_some'. Advantage: none, disadvantage: same as above.

  • Firing directly 'init.some' should fire callbacks under 'init.some' and 'init.some.more'. Still the same as firing the callbacks in 'init_some' and 'init_some_more' separately with the same advantages and disadvantages.
  • Setting the namespace to 'other' or firing directly 'init.other' should fire the callbacks registered under 'init.other'. That's exactly the same as simply firing 'init_other'.

So the advantage of using namespaced vs. "flat" actions is a bit shorter code, the disadvantages are: more complex API, different than the PHP actions (that's a big one), no control of child actions firing order.

Another possibility is to add namespacing support in the API and never use it in core. Usually the core code does doAction and applyFilters, and plugins do addAction and addFilter, so if core never uses action namespaces, the plugins wouldn't need to deal with the differences from the PHP API.

I agree in saying that we should keep the actions/filters in separate containers.

Yes, they either have to be in separate containers or we can have a flag in each object, perhaps something like isFilter: true/false. Both would work well and require about the same amount of code.

#34 in reply to: ↑ 33 @mikeschinkel
5 years ago

Replying to azaozz:

Perhaps we should consider all pros and cons. For example, lets say we have an action 'init' with several namespaces/children: 'init.some', 'init.some.more', 'init.other'.

Keep it very simple, like the original wiki:

"What's the simplest thing that will work?"

If there have ideas we might want to implement, like using periods for namespacing, then enforce a convention where hook names can't contain periods in the first implementation. Then wait and see how people actually use JavaScript actions and filters before making them more advanced based on speculation.

Better to get something simple into 3.5 than waiting who knows how many versions for perfection. Shipping is a feature. :)

#35 @CaptainN
5 years ago

I added namespace support to SignalsLite.js (all unit tested):

It works like this:

// setup in core somewhere, or elsewhere
var actions = {
    // simple factory method
    addAction: function( name ) {
        this[ name ] = new SignalLite( this );
    }
}

// wp core or someone else, adds the action hooks
actions.add( 'wpAction' );

actions.wpAction.ns.add( 'myNamespace' );

// now the namespaces actions can be used
actions.wpAction.myNamespace.add( function() {
    // do something with the action    
} );

// later when it comes time to cleanup:
actions.wpAction.myNamespace.remove();

I didn't add a way to dispatch a Signal only to a namespaced sub-set of listeners, because it didn't seem to make sense to expose that kind of functionality. If a listener was added to a Signal, it seems inappropriate to only send the signal to certain listeners, it adds a second level of subscriber sets, and that can be abused to basically duplicate the dispatcher functionality, where there should probably be a second named action - it may be better to encourage the creation of another distinct hook.

I think event namespaces are mostly useful as a tool for plugin developers to use to be able to clean themselves up, without having to retain a reference to all the listeners they may add all over.

jQuery does support namespaced dispatching, and if there was a strong desire for namespaced dispatching, it could be added easily to SignalsLite.js. I'm not sure how often you'd have plugin or extension developers triggering or dispatching actions and filters (like they frequently do in jQuery), so it may not really be an issue, unless it's wanted for core use

Edit: I used the wrong markup format. heh

Last edited 5 years ago by CaptainN (previous) (diff)

#36 @koopersmith
5 years ago

Some comments on the wp-hooks implementation in IRC.

#37 follow-up: @CaptainN
5 years ago

SignalsLite.js now has every feature in the ticket (and then some, like safe dispatching, addToTop, once, eachReturn handler for filtering) - including priorities, namespaces and a way to cancel dispatch. And, it's incredibly fast (crazy fast!). I did end up moving the safe dispatcher out to it's own sub class (SignalDispatcher), because it was big, relies on the DOM, and is frankly, not the fasted thing in the world (it's worth it some cases! but maybe not every case). In its place is a simple trigger method, which will just error out (like jQuery) if there's an error in a listener.

Anyway, now that that base is done, maybe I'll take a stab at putting the actual WordPress part around it - if there is any interest (I'm not sure how far along any other implementations are).

Here's what the API might look like with Signals - you could keep things string based if you wanted, and still expose the Signal feature set (which I think has some advantages compared with period based namespacing, or the full set of add methods you get with Signal priorities). Here's a quick example of what could be done:

// add hook with chaining
// everything after addAction() is implemented and unit tested in SignalsLite.js
$wp.hooks.addAction( "actionname" )
    .ns( "myNamespace" )
        .add( function() {} );

// additional adds can be property based (they don't have to be though)
$wp.hooks.actionname.myNamespace.add( function() {} );

// here's what priorities look like
$wp.hooks.addAction( "actionname" )
    .priority( 10 )
        .addToTop( function() {} );

Edit: Er, SignalsLite.js has every feature in the ticket required by the dispatcher component. It still doesn't have the WP specific stuff - the filter/action structure, or the ability to track which ones have already been triggered, etc. That'd need to be implemented on top of SignalsLite.js.

Last edited 5 years ago by CaptainN (previous) (diff)

#38 @iandunn
5 years ago

  • Cc ian_dunn@… added

#39 @mitchoyoshitaka
5 years ago

  • Cc mitcho@… added

#40 @husobj
5 years ago

  • Cc ben@… added

#41 @toscho
5 years ago

  • Cc info@… added

#42 in reply to: ↑ 37 ; follow-up: @webord
5 years ago

  • Cc webord.net@… added

Replying to CaptainN:
Question, what's the status on this ticket? I would love to check the code you guys have been working with.

#43 in reply to: ↑ 42 @CaptainN
5 years ago

Replying to webord: I haven't done much in the way of a patch. I mostly was focusing on how to do the actual dispatching, and figured a lot of the feature requirements actually belong in that realm. It was actually a fun way to get back into JavaScript after a couple years on other platforms. You can find all the SignalsLite.js stuff here:

https://github.com/CaptainN/SignalsLite.js

It's pretty robust at this point, and there are unit tests for it. I wasn't sure of the interest level in using SignalsLite.js, and it looks like there may already be another dispatch infrastructure.

This is from the IRC link:
https://github.com/carldanley/WordPress-Action-Filter-Library/blob/master/wp.hooks.js

Anyway, I don't own this ticket. You'd have to as the WP folks for more status info. :-)

#44 @nacin
5 years ago

  • Milestone changed from 3.5 to Future Release
  • Type changed from task (blessed) to feature request

#45 @kadamwhite
5 years ago

  • Cc kadamwhite added

#46 @juliobox
5 years ago

  • Cc juliobosk@… added

#47 @technosailor
5 years ago

  • Cc aaron@… added

#48 @alexkingorg
5 years ago

  • Cc public@… added

#49 @Bueltge
5 years ago

  • Cc frank@… added

#50 @aaroncampbell
5 years ago

  • Cc aaroncampbell added

#51 follow-up: @scribu
5 years ago

  • Keywords 2nd-opinion added

Is this still needed, given that Backbone is bundled into Core since 3.5? Backbone.events seems to be good enough.

Basic usage:

var events = _.clone(Backbone.events);

events.on('some_action', some_callback);

// later

events.trigger('some_action', 'some_data');

#52 in reply to: ↑ 51 ; follow-ups: @azaozz
5 years ago

Is this still needed...

The original idea was to have add_action(), add_filter(), do_action(), etc. in JS that behave similarly to PHP. Unfortunately neither Backbone.events nor jQuery.Callbacks() have a way to specify callback priority. Also missing did_action() which might be useful to have.

#53 in reply to: ↑ 52 @vhauri
5 years ago

The original idea was to have add_action(), add_filter(), do_action(), etc. in JS that behave similarly to PHP. Unfortunately neither Backbone.events nor jQuery.Callbacks() have a way to specify callback priority. Also missing did_action() which might be useful to have.

+1 to what @azaozz said...the priority of execution is important in the use-cases I have (or at least the ability to perform a did_action()-type call to check if another action has fired).

#54 @azaozz
5 years ago

A really nice (super fast and simple to use) implementation of add_action(), do_action(), etc. in JS: https://github.com/carldanley/WP-JS-Hooks

#55 @stephenharris
5 years ago

  • Cc stephenharris added

#56 @Jesper800
5 years ago

  • Cc jeve0@… added

#57 @sennza
5 years ago

  • Cc bronson@… added

#58 @jblz
5 years ago

  • Cc jeff@… added

#59 @sc0ttkclark
5 years ago

  • Cc lol@… added

#60 @blepoxp
4 years ago

  • Cc glenn@… added

#61 @adamsilverstein
4 years ago

  • Cc adamsilverstein@… added

#62 @westonruter
4 years ago

  • Cc weston@… added

#63 follow-up: @adamsilverstein
4 years ago

this would be a great feature to work on for 3.7!

#64 in reply to: ↑ 63 @carldanley
4 years ago

Replying to adamsilverstein:

this would be a great feature to work on for 3.7!

Couldn't agree more. I think there is a lot of potential for something like this in core. Filters come in super handy in JS when building an application that has a lot of different components. The library I wrote is pretty well optimized. I still need to tweak a few small things and cleanup some code BUT I think it's pretty solid.

There was some good talk about whether or not we could implement a solution that just extends jQuery to support the filters needed. I would love to continue working on this piece however possible; maybe even squeeze it into 3.7.

#65 @MikeSchinkel
4 years ago

Since WordPress has added Backbone.js as an important component of core, maybe the best approach is to adopt (an extension of) Backbone.Events for this?

#66 @carldanley
4 years ago

I've thought about that many times over and the fact I keep coming back to is that we'd need to enqueue backbone on the frontend as well (for things like comments, etc). I'm ok enqueueing Backbone wherever needed as I use it for all of my applications but more often than not, most plugins aren't using backbone and so they wouldn't make use of it at all if it were enqueued. Also, it's not a heavy library so it's not a huge determining factor.

We can extend things really easily when needed, I'm just trying to think of the most efficient way to make this available to everyone without a ton of dependencies.

#67 in reply to: ↑ 52 ; follow-up: @MikeSchinkel
4 years ago

Replying to azaozz:

The original idea was to have add_action(), add_filter(), do_action(), etc. in JS that behave similarly to PHP. Unfortunately neither Backbone.events nor jQuery.Callbacks() have a way to specify callback priority. Also missing did_action() which might be useful to have.

Saw this after posting today.

Rather than dismissing Backbone.Events you might want to consider extending them as Backbone.Wreqr does. It would not be too difficult to add priority and did_action() on top of Backbone.Events, and it's even possible someone else has already written most (all?) of the needed extensions.

#68 in reply to: ↑ 67 ; follow-up: @carldanley
4 years ago

Replying to MikeSchinkel:

Rather than dismissing Backbone.Events you might want to consider extending them as Backbone.Wreqr does. It would not be too difficult to add priority and did_action() on top of Backbone.Events, and it's even possible someone else has already written most (all?) of the needed extensions.

Just to clarify, I'm not dismissing it rather just saying that it becomes cumbersome. It's a bigger decision to extend Backbone because we then need to include Backbone & underscore everywhere in core (frontend & backend) to make this events implementation available. If that's the route people are leaning to, that's fine with me.

You can easily extend Backbone.Events when it's enqueued, to provide Backbone.Events the additional functionality this library would offer. Curious to hear thoughts from nacin and azaozz about this.

Last edited 4 years ago by carldanley (previous) (diff)

#69 in reply to: ↑ 68 @MikeSchinkel
4 years ago

Replying to carldanley:

Just to clarify, I'm not dismissing it rather just saying that it becomes cumbersome.

I was really just replying to the ticket vs. replying to your posts specifically. I wasn't ignoring your comments on purpose; your comments just caused me to see the ticket in my inbox again. I've been working with Backbone.js since I last commented on this ticket and now had some perspective to share.

It's a bigger decision to extend Backbone because we then need to include Backbone & underscore everywhere in core (frontend & backend) to make this events implementation available. If that's the route people are leaning to, that's fine with me.

That's a good point. I'm obviously not in the position to decide any of that.

What I have noticed is that the Events subset of Backbone is both elegant and small, and could be forked to load for when full Backbone is not loaded plus extended in a way that would be compatible with an event-only fork as well as full Backbone. FWIW.

BTW, the more I learning about the Javascript features used to create Backbone.js, the less I like programming in PHP.

#70 @lgedeon
4 years ago

I think a more compact solution that does not rely on backbone would provide better stability going forward. It could be done as a fork of backbone or designed to be compatible with it, though. Just saying, if many plugins are going to depend on it, we probably need to own it.

I am thinking something like Carl's code, if not a bit more compact, could become a very baked-in part of core code that doesn't have to be reworked or replaced as we add and remove other libraries.

Last edited 4 years ago by lgedeon (previous) (diff)

#71 @azaozz
4 years ago

...something like Carl's code could become a very baked-in part of core code that doesn't have to be reworked or replaced as we add and remove other libraries.

Agreed. Also note that for hooks to be effective the JS has to be the first thing that loads in the head (so the hooks can be used at any time). Having an external dependency is not ideal for this.

#72 follow-up: @carldanley
4 years ago

Dropping a note here for future reference: changed the lib at https://github.com/carldanley/WP-JS-Hooks to remove a lot of NFE's. Also changed the module pattern to a revealing module pattern. All tests pass.

#73 in reply to: ↑ 72 ; follow-up: @adamsilverstein
4 years ago

Replying to carldanley:

Dropping a note here for future reference: changed the lib at https://github.com/carldanley/WP-JS-Hooks to remove a lot of NFE's. Also changed the module pattern to a revealing module pattern. All tests pass.

I agree with not using Backbone unless we need it, and I love Backbone! I looked at your code very briefly and that got me excited - it looks like you are a long ways toward a solution for Core! I hope you will have some time to fine tune and test in the next two months, we could use your expertise.

#74 in reply to: ↑ 73 ; follow-up: @carldanley
4 years ago

Replying to adamsilverstein:

I hope you will have some time to fine tune and test in the next two months, we could use your expertise.

For sure! I'm watching this thread pretty closely. Just let me know however I can help.

#75 in reply to: ↑ 74 ; follow-up: @adamsilverstein
4 years ago

Replying to carldanley:

Replying to adamsilverstein:

I hope you will have some time to fine tune and test in the next two months, we could use your expertise.

For sure! I'm watching this thread pretty closely. Just let me know however I can help.

i may miss this wednesday's meeting in irc (travel day), but hoping we can form a 'JavaScript team' for 3.7 - in addition to Hooks & Filters, I'm hoping we can start the ball rolling on getting JavaScript unit tests into core - currently there is a strong push for PHP unit tests everywhere, and we need JS unit tests. See #24870.

#76 @swissspidy
4 years ago

  • Cc hello@… added

#77 @beaulebens
4 years ago

  • Cc beau@… added

#78 @bradt
4 years ago

  • Cc brad@… added

#79 @itsananderson
4 years ago

  • Cc will@… added

#80 @buffler
4 years ago

  • Cc jeremy.buller@… added

#81 @Chouby
4 years ago

  • Cc frederic.demarle@… added

#82 in reply to: ↑ 75 @withinboredom
4 years ago

Is there any update to this? I'm working on a overhauling a plugin's javascript and could really use this in core -- if there's someone working on it, I would love to help out and provide real world examples. And if not, I'm more than willing to get it started.

This ticket was mentioned in IRC in #wordpress-dev by azaozz. View the logs.


4 years ago

#84 @gcorne
4 years ago

I am just now trying to get caught up with all the thinking that has gone into this. And, I still need to spend some time looking at and playing around with the fine work that Carl has done (Yay, for unit tests!). Here are few initial thoughts:

  • In the javascript land, we should probably consider garbage collection. In cases where we are dealing with short-lived “view” code that has references to DOM nodes, it would be nice if subscribers could easily clean up references to the event bus. This is the reason that Backbone added the listenTo and stopListening methods to Backbone.Events which is mixed in to all the other Backbone constructors.
  • I think we would want to support a context parameter so that when binding a callback the subscriber code can control what this refers to in the callback. We also need to consider how to best handle context on the publisher side. Do we want to support an arbitrary set of parameters that are then passed to the subscriber callback like the PHP plugin API supports?
  • I am not super up on benchmarks for the jQuery event system vs. others, but one thing that I don’t like about jQuery is that it is hard to inspect the catalog of registered subscribers to events. I think that it should be pretty easy to look at the data structure holding all the event bindings.
  • If we are going to create a dependency, I think it might be best for the dependency to be on underscore.js, which is only 4.7k compressed and includes lots of useful stuff for building a robust event/filter system.
  • Building on the idea of using underscore, we should build the core library so that it can be used under a separate namespace, so a core feature or a plugin could do something like:
wp.plugins.myAwesomePlugin.hooks = _.extend( {}, wp.Events );
  • In terms of style, I am not 100% sure how close it needs to match the PHP API, especially as the role of “front-end engineer” and the role’s heavy javascript skill set becomes more commonplace.
  • Has anyone looked through core for places that are good candidates for adding some hooks? It might be worth considering some specific use cases.

#85 @jeremyfelt
4 years ago

  • Focuses javascript added

#86 follow-up: @azaozz
4 years ago

In my opinion the biggest advantages of having a dedicated hooks API are:

  • Similarity to the PHP hooks. It's true that more and more (plugin) developers understand JS better, but there are a lot that will find this very desirable.
  • Interoperability. None of the event APIs we can use allow a callback to be scheduled to run before or after another callback, even before the second callback is defined. In the PHP actions and filters this is done with 'priority'. It has its limitations but has been working really well for about 10 years. Considering that some hooks would be used by core plus 2-3-4 plugins, having priority would be great.

Has anyone looked through core for places that are good candidates for adding some hooks?

Heartbeat works only with hooks (both in PHP and JS), now autosave has several hooks too.

I think we would want to support a context parameter

Agreed, having a scope can be quite handy and will make this better.

If we are going to create a dependency...

Don't think we need a dependency. The current patch is fast, simple and works very well.

...we should build the core library so that it can be used under a separate namespace

This may be handy in some cases however there are already jQuery custom events, jQuery callbacks, and Backbone events that can be used.

...it would be nice if subscribers could easily clean up references to the event bus. This is the reason that Backbone added the listenTo and stopListening

Yes, I see this rather as supporting removing hooks in "bulk" like jQuery's $(document).off('.my-hooks').

Last edited 4 years ago by azaozz (previous) (diff)

#87 @adamsilverstein
4 years ago

I keep coming across the desire to add JS hooks to core. Overall I would prefer functionality that closely mirrors the way we do hooks in PHP - carldanley's github code looks good and I would love to see this in core so we can start adding some hooks and testing!

If there is general consensus that this makes sense I would be happy to work up a patch to add the code (including unit tests) to core.

#88 in reply to: ↑ 86 ; follow-up: @gcorne
4 years ago

Considering that some hooks would be used by core plus 2-3-4 plugins, having priority would be great.

The ability to control execution order is definitely worth having.

Agreed, having a scope can be quite handy and will make this better.

I went ahead and had a go at adding a context parameter. See https://github.com/gcorne/WP-JS-Hooks/commit/e7ab26d9aae2f5383b8ec156454ce241bcb4a87e

I also think that we should incorporate the ability to remove a specific hook based on === comparison. Something like:

var func = function() {
    // do awesome stuff
};

wp.hooks.addAction( 'foo', func );
// lots of code

wp.hooks.removeAction( 'foo', func );

We could either make the callback reference optional in removeAction() and when not passed remove all, or perhaps create a separate method removeAllActions() .

#89 in reply to: ↑ 88 @carldanley
4 years ago

Replying to gcorne:

The ability to control execution order is definitely worth having.

+1.

I went ahead and had a go at adding a context parameter. See https://github.com/gcorne/WP-JS-Hooks/commit/e7ab26d9aae2f5383b8ec156454ce241bcb4a87e

+1. I left comments - just needs adjusted documentation.

I also think that we should incorporate the ability to remove a specific hook based on === comparison.
We could either make the callback reference optional in removeAction() and when not passed remove all, or perhaps create a separate method removeAllActions() .

+1 on this; would love to see a patch come through!

#90 @gcorne
4 years ago

I went ahead and added the ability to remove specific callbacks in https://github.com/gcorne/WP-JS-Hooks/commit/c109d4f4cd19fc22d89779cfa1a9ef3edcea6ee0.

One other enhancement that I think needs to before adding to core is support for jQuery-like namespacing to make cleaning up hooks easier, although since we don't have to deal with built-in event names, I wonder if we should be using a convention that is a bit less confusing. Appending a namespace (e.g. event.namespace seems a little backwards to me.

@TV productions
4 years ago

Implemented the wp hooks from https://github.com/gcorne/WP-JS-Hooks/

#91 @TV productions
4 years ago

In 21170-1.patch:

  • Implemented the last WP JS Hooks from gcorne.
  • Replaced $(document).trigger() for autosave.js, heartbeat.js, word-count.js, post.js, inline-edit-post.js, tinymce/plugins/wordpress/plugin.js and wp-auth-check.js.
  • I've tested it, but you should test it too.

Why I implemented this? Because I really like the idea of js hooks and this solution of gcorne seems good for now, because it looks a lot like the PHP API, so probably a lot developers will recognise it. Besides that it has no dependencies, what is nice for front-end (as you guys noted earlier).

#92 @TV productions
4 years ago

Hmm, forgot to add script-loader.php to 21170-1.patch.
In 21170-2.patch:

  • Extended 21170-1.patch
  • Added wp-hooks.js as dependency of heartbeat.js, autosave.js and word-count.js (among others).
Last edited 4 years ago by TV productions (previous) (diff)

#93 @TV productions
4 years ago

  • Keywords has-patch dev-feedback added

#94 @PeterRKnight
3 years ago

No love for has_action/did_action equivalents?

This ticket was mentioned in Slack in #core by azaozz. View the logs.


2 years ago

#96 @justnorris
2 years ago

Still no love for this? It looks to me like everything is ready for this to be pulled in.
I'm using WP-JS-Hooks in all my Wordpress themes now, and I see that Elliot is using them in Advanced Custom Fields 5 as well.

WP-JS-Hooks is awesome and battle tested. Why isn't this moving forward ?

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


2 years ago

#98 follow-up: @adamsilverstein
2 years ago

  • Owner changed from koopersmith to adamsilverstein
  • Status changed from new to assigned

I am going to work up a refreshed patch for including @carldanley's hook library into core. This small bit of additional code could have a big impact on the future of JavaScript development with WordPress.

#99 in reply to: ↑ 98 @azaozz
2 years ago

Replying to adamsilverstein:

Yeah, it's sad we couldn't get this done at the time (in 3.5-3.6). There were only a few $( document ).trigger() "hooks" in core at the time, now there are many more and we will probably have to keep them for back-compat. I was still hoping koop will eventually have another look at it some day and change his mind...

I still think there are clear advantages in having a global events bus where core and plugins can easily (and orderly) interact with each other. This is in addition to separate components using separate "events systems".

Not sure if this needs to be usable in other scopes. We use JS libraries for all bigger components and it seems best to use the specific library's events internally in the component, and use the global bus for globally accessible "hooks".

#100 @adamsilverstein
2 years ago

  • Keywords 2nd-opinion removed

In 21170.diff:

Add the events library created by @carlcanley in this GitHub Repo (renamed as wp-hooks for consistency.

Merging this code will make no change in core actions yet. We can add them going forward and use a helper wrapper to update existing events for backwards compatibility (so existing document.trigger's still get fired). I wanted to keep the patch as small as possible. It would make sense to add repo unit tests to core as well when merging, I can work on adding that to the patch.

With this patch, plugins, themes and other core patches could start using wp-hooks by simply adding 'wp-hooks' as a script dependency in wp_enqueue_script or enqueueing directly with wp_enqueue_script( 'wp-hooks' );

Notes from the GitHub repo:

### Dependencies
WP-JS-Hooks does not require any third-party applications or software. The library is completely self-maintained. However, the repository itself uses Grunt to perform tasks such as JSHint, Uglify, and QUnit.

### API Usage
API functions can be called via the global wp.hooks like this wp.hooks.addAction(), etc.

  • addAction( 'namespace.identifier', callback, priority )
  • addFilter( 'namespace.identifier', callback, priority )
  • removeAction( 'namespace.identifier' )
  • removeFilter( 'namespace.identifier' )
  • doAction( 'namespace.identifier', arg1, arg2, moreArgs, finalArg )
  • applyFilters( 'namespace.identifier', content )

### Features

  • Fast and lightweight, only 1.3kb
  • Priorities system ensures hooks with lower integer priority are fired first.
  • Uses native object hash lookup for finding hook callbacks.
  • Utilizes insertion sort for keeping priorities correct. Best Case: O(n), worst case: O(n2)

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


2 years ago

#102 follow-ups: @atimmer
2 years ago

I stumbled upon this ticket and because you haven't received any feedback I thought I could give some.

  • The JSDoc in the patch misses type declarations.
  • The wholeMethodsAvailable is weird readability wise. It works because the declaration of the variable is hoisted to the top, then the functions are hoisted and then MethodsAvailable is set to the correct value. I think it's better to be explicit that that's going on so to fix that add a var MethodsAvailable on top and moving the assignment to the bottom.
  • If the first argument of doAction is required shouldn't it just be placed in the first position in the function declaration? Same goes for applyFilters.
  • It is possible to document that a function accepts a list of arguments using 3 dots: http://usejsdoc.org/tags-param.html, so for doAction: @param {...}
  • There is a space missing before the first ( of all if statements.
  • _removeHook doesn't need a context argument because it's never called with a context. This also makes the function cleaner.
Last edited 2 years ago by atimmer (previous) (diff)

#103 in reply to: ↑ 102 @adamsilverstein
2 years ago

Replying to atimmer:

I stumbled upon this ticket and because you haven't received any feedback I thought I could give some.

Thanks for the feedback. I will work on your suggestions in github and come back with an updated patch. I will also consider spinning this off as a plugin to allow others to easily test it. We can hook into all of the existing document.trigger actions there as well.

This ticket was mentioned in Slack in #core-restapi by jeremyfelt. View the logs.


12 months ago

This ticket was mentioned in Slack in #core-restapi by matt. View the logs.


12 months ago

#106 in reply to: ↑ 102 @adamsilverstein
12 months ago

Replying to atimmer:

Getting back to working on your feedback here:

In 21170.2.diff

  • Add JSDoc type declarations
  • improve @param descriptions, including spread notation
  • add a missing space before the first ( of all if statements.

#107 @adamsilverstein
12 months ago

21170.3.diff:

  • Improve readability of MethodsAvailable by moving it to the bottom
  • remove context from _removeHook

#108 @adamsilverstein
12 months ago

21170.4.diff Includes some code fixes and also bundles the original qunit tests

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


10 months ago

This ticket was mentioned in Slack in #core by kadamwhite. View the logs.


10 months ago

#112 follow-up: @aduth
9 months ago

Joining this discussion late in the conversation, so forgive me in advance if I've failed to adequately come up to speed here in my remarks. @adamsilverstein, the approach in your latest patches is pretty excellent. I went through the latest in detail. Below are a few of my specific observations:

  • I don't see the value in supporting chaining. I notice there's been some prior discussion here on its usefulness. Could someone provide a practical example where it would be useful?
  • Similarly, why do we expose context as an argument? this is a miserable keyword in JavaScript. At worst, if the developer wanted to leverage it, they could use Function#bind or _.bind on the callback argument.
  • Minor: A handful of violations of negation spacing guideline "Any ! negation operator should have a following space.*" (particularly _removeHook)
  • Could we generalize implementations between actions and filters to a common creator functions? e.g. roughly createHookAdder = ( type ) => ( name, callback, priority ) => _addHook( type, name, callback, priority )
  • Your _addHook "prop itself" logic, while more performant, opens you up to edge cases around object prototype lookup, e.g. wp.hooks.addFilter( 'valueOf', function() {} ) will throw an error. hasOwnProperty addresses this, or you can consider creating the hook storage with an empty prototype via Object.create( null ) (browser support varies)
  • Seems odd to me that _runHook is generalized while its implementation then considers very specifically the type in how it's executed. Instead seems applyFilters and doAction should just contain their respective logic.

To the first two points, I think perhaps we've tried to provide more options than are really useful, which adds complexity to the implementation and breaks alignment with my understanding of the equivalent PHP functions (where I'd expect these functions to be used nearly identically).

#113 @aduth
9 months ago

Also the current logic wouldn't allow a priority of 0, because parseInt( ( priority || 10 ), 10 ) results in 10 (0 being falsey).

@aduth
9 months ago

My take on @adamsilverstein's patch with consideration of previous remarks

@aduth
9 months ago

(Fixed 21170.5.diff)

#114 @adamsilverstein
9 months ago

@aduth thanks for your input, testing and refreshed patch. I will review and test soon.

#115 in reply to: ↑ 112 @azaozz
9 months ago

Replying to aduth:

21170.5.2.diff looks good. Couple more things to consider:

  • Do we want to do "jQuery style namespacing" or something similar? The problem is removing particular callback from a filter in PHP is very easy as the "identifier" is usually a string (even when it is a method). This is not the case in JS where a callback cannot be removed if there is no reference to it. If not, should we add some easy way to remove specific callbacks?
  • In PHP there are doing_action(), did_action(), and has_action(). They are somewhat useful in some cases. Do we need equivalents in JS?

#116 @aduth
8 months ago

Replying to azaozz:

Do we want to do "jQuery style namespacing" or something similar? The problem is removing particular callback from a filter in PHP is very easy as the "identifier" is usually a string (even when it is a method). This is not the case in JS where a callback cannot be removed if there is no reference to it. If not, should we add some easy way to remove specific callbacks?

I dunno, seems more like a "nice to have" than a strictly useful feature. Probably a decision better driven by real use-cases, but I suspect in most situations I'd want to remove a hook, it'd be within the same scope as a reference to the original function anyways. To me, jQuery namespaces are most valuable for unbinding from a number of different event names simultaneously, something that wouldn't seem a common requirement for hooks.

In PHP there are doing_action(), did_action(), and has_action(). They are somewhat useful in some cases. Do we need equivalents in JS?

These seem simple enough to implement and maintain that I wouldn't see much of a downside to including them even if just for benefit of aligning APIs between PHP and JS hooks.

#117 @adamsilverstein
8 months ago

  • Keywords has-unit-tests added; dev-feedback removed

In 21170.6.diff :

  • Builds on last patch from @aduth
  • Add wp.hooks.doingAction, didAction and hasAction as @azaozz's suggested
  • new functions match behavior of php functions - note that didAction returns a run count or 0 for un-run, not a boolean. After removing a hook, the count resets to 0.
  • Tracks the current (last) action/filter running
  • Adds tests for these new helpers
  • Ensure hooks completely removed by removeAction/removeFilter

Working on some use case examples next.

@adamsilverstein
8 months ago

fixes for jshint/travis

#118 @adamsilverstein
8 months ago

21170.7.diff includes a couple of fixes to make jshint happy and get travis:js tests to pass.

This ticket was mentioned in Slack in #core by rmccue. View the logs.


7 months ago

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


7 months ago

This ticket was mentioned in Slack in #core by jnylen. View the logs.


6 months ago

#122 @adamsilverstein
6 months ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests removed

Noticed a couple other PHP hook functions we should consider matching in wp.hooks (and adding some tests): remove_all_actions(), doing_filter(), remove_all_filters(), current_filter() and has_filter().

#123 @adamsilverstein
5 months ago

In 21170.8.diff:

  • slight code cleanup
  • stub out removeAll actions
  • add doingFilter, currentFilter, hasFilter.
  • should we add a doingAction, would return last action filred?

#124 @adamsilverstein
5 months ago

21170.9.diff

  • fix some typos/bugs
  • add QUnit tests for doingFilter, didFilter and hasFilter.

This ticket was mentioned in Slack in #core-js by azaozz. View the logs.


4 months ago

#126 @adamsilverstein
4 months ago

  • Keywords commit has-patch added; needs-patch needs-unit-tests removed
  • Milestone changed from Future Release to 4.9

In 21170.10.diff:

Complete support for removeAllActions( 'namespace.identifier' ) & removeAllFilters( 'namespace.identifier' ) as well as adding tests for these functions.

#127 @adamsilverstein
4 months ago

In 21170.11.diff: pass around the actual hooks array objects instead of a string, potentially making the code a little more efficient and easier to read, see https://github.com/WordPress/packages/pull/13#issuecomment-316392090

#128 @adamsilverstein
4 months ago

21170.12.diff is a webpack build of the wp.hooks module, from ongoing work in https://github.com/WordPress/packages/pull/13

Changes:

  • To remove all callbacks hooked on a name, call removeAllFilters/Actions( 'name' ) - removeFilter/Action( 'name' ) no longer does that and now expects a callback to match against.
  • Docs cleanup
  • hooked items sorted by priority as they are added, eliminating the need to sort the hooks.
  • tests are rewritten in jest and future development will happen in the packages github.
  • we should be able to release this as an npm module.

Thanks to nylen for all his work improving this.

#129 follow-up: @azaozz
4 months ago

Are we adding support for "named" actions/filters: addAction( 'tag.my-name', function() ... ) as per the last chat in #core-js? Thinking we should make that mandatory, i.e. all actions and filters will have to be "named" for easy interoperability.

#130 in reply to: ↑ 129 ; follow-up: @jnylen0
4 months ago

  • Keywords commit removed

In general this is a surprisingly complicated bit of code to get right. See https://github.com/WordPress/packages/pull/13#issuecomment-316843679 for a (non-exhaustive) list of things that still need to be done. It's roughly 1,000,000x easier to iterate on it in GitHub, so please comment on the specific approach there.

Replying to azaozz:

Are we adding support for "named" actions/filters: addAction( 'tag.my-name', function() ... ) as per the last chat in #core-js? Thinking we should make that mandatory, i.e. all actions and filters will have to be "named" for easy interoperability.

Yes, this needs to go in.

However, I'm not sure we should make it mandatory. And if we do make it mandatory, then it should be a separate argument (at that point there's no reason to include it in the "hook name" argument and then have to split apart that string).

#131 in reply to: ↑ 130 ; follow-up: @azaozz
4 months ago

Replying to jnylen0:

However, I'm not sure we should make it mandatory.

Thinking it should be mandatory so we have parity with the way hooks work in PHP. A plugin should always be able to remove an action or a filter, also when it is added by another plugin. If the callback name is not mandatory some plugins will probably skip it. Then we will have to do "Doing it wrong..." etc. again.

Working example:

function my_func() {
    remove_filter( 'the_content', 'wpautop' );
}

add_action( 'init', 'my_func' );

This is very straightforward, a plugin removes a core filter. Then another plugin can do:

function my_func_2() {
    remove_action( 'init', 'my_func' );
}

add_action( 'init', 'my_func_2', 1 );

Being able to do these things with all hooks is very important for both core and plugins.

...it should be a separate argument (at that point there's no reason to include it in the "hook name" argument and then have to split apart that string).

I don't have strong preference here. Being in the "hook name" follows the jQuery pattern that is very well known. However this is optional in jQuery. Making it a second argument will convey better that it is required.

Last edited 4 months ago by azaozz (previous) (diff)

#132 in reply to: ↑ 131 ; follow-up: @jnylen0
4 months ago

Replying to azaozz:

Thinking it should be mandatory so we have parity with the way hooks work in PHP. A plugin should always be able to remove an action or a filter, also when it is added by another plugin. If the callback name is not mandatory some plugins will probably skip it. Then we will have to do "Doing it wrong..." etc. again.

In the plugins repo there are about 6500 instances of plugins adding anonymous functions to PHP filters or actions. These are currently impossible for other plugins to remove. Is this something we want to prevent for JS filters?

Maybe, but sacrificing the very natural JavaScript pattern add_filter( 'some_filter_name', function() { ... doesn't really seem worth it to me.

I'm in favor of requiring that core JS filters always have a namespace or function name, but not so sure about making these mandatory for plugins. I think it would add a significant amount of friction for a fairly minor use case.

Either way, PHP parity is not something we should necessarily strive for because the two languages are very, very different. Here's an example of an alternative approach that would allow the same functionality in a way that is fairly natural in JavaScript, but impossible in PHP:

forEachFilter( 'filter_name', function( callback, priority, namespaces ) {
    // Inspect `callback` (including looking at the function name and
    // maybe even its code) and call `removeFilter` if necessary
} );

#133 in reply to: ↑ 132 ; follow-up: @azaozz
4 months ago

Replying to jnylen0:
Generally I think lambda functions suck when you're writing code that is supposed to be easy to extend.

In the plugins repo there are about 6500 instances of plugins...

That may be but these plugins are already incompatible with all WordPress installs (no closures support in PHP 5.2).

Maybe, but sacrificing the very natural JavaScript pattern add_filter( 'some_filter_name', function() { ... doesn't really seem worth it to me.

If you feel strongly about that, lets go with the jQuery pattern instead of a separate arg. See my previous comment.

I'm in favor of requiring that core JS filters always have a namespace or function name, but not so sure about making these mandatory for plugins. I think it would add a significant amount of friction for a fairly minor use case.

Then we will have many hook that are removable and some that are not? Don't think "partial support for removing actions and filters" is a good thing. Having said that, there may be a need for non-removable hooks, both for core and plugins. However if we are going to implement that I'd rather see it as another arg instead of an optional "namespace", mostly for clarity/readability.

Either way, PHP parity is not something we should necessarily strive for...

I agree. I don't really care about PHP or JS or any other particular language. I just want to keep features that have proven useful over the years, and maybe improve them a bit if possible.

Being able to control which actions and filters run at a particular time is a pretty important thing imho. Also, I'm tired of having to add "Doing it wrong.." in more and more places. All of these usually indicate some requirement that was missed when implementing a feature. Look at shortcodes... :)

Last edited 4 months ago by azaozz (previous) (diff)

#134 in reply to: ↑ 133 @jnylen0
4 months ago

Replying to azaozz:

Being able to control which actions and filters run at a particular time is a pretty important thing imho. Also, I'm tired of having to add "Doing it wrong.." in more and more places. All of these usually indicate some requirement that was missed when implementing a feature. Look at shortcodes... :)

Ok... I'm thinking for the initial version we can try something like this:

  • Implement jQuery-style namespaces.
  • Require either a namespace or a function with a name.

We can always take away restrictions later; adding new restrictions is more problematic.

#135 follow-up: @adamsilverstein
4 months ago

Thanks for the discussion & work here!

A plugin should always be able to remove an action or a filter, also when it is added by another plugin.

Note that we have removeAllFilters( 'filtername' ) and removeAllActions( 'filtername' ) which enables removing all hooked callbacks (without knowing what they are)...

Implement jQuery-style namespaces.
Require either a namespace or a function with a name.

Both sound good if we can figure out how to enforce that requirement.

#136 in reply to: ↑ 135 ; follow-up: @azaozz
4 months ago

Replying to jnylen0:

We can always take away restrictions later; adding new restrictions is more problematic.

Exactly. Adding restrictions after an API has been released and used by plugins is pretty much impossible in WordPress.

Replying to adamsilverstein:

Note that we have removeAllFilters( 'filtername' ) and removeAllActions( 'filtername' ) which enables removing all hooked callbacks (without knowing what they are)...

Yes, but that's not that useful, can even be quite "dangerous". You'll never know what hooks will be removed. Imagine what would happen if all hooks from 'init' are removed :)

Implement jQuery-style namespaces.
Require either a namespace or a function with a name.

Both sound good if we can figure out how to enforce that requirement.

Yeah, been thinking about that too. It's not hard to check for a dot in the hook name or if the callback is a string. However for ease of use/clarity it would probably be better to always require a jQuery-style namespace. I.e. all callbacks will have to be "named" in the hook name. The chars for that "callback name" should also be restricted imho. Something like /[a-zA-Z0-9_-]+/ should work well. That takes care of several things at the same time: sanitization, readability, etc.

Last edited 4 months ago by azaozz (previous) (diff)

#137 in reply to: ↑ 136 ; follow-ups: @jnylen0
4 months ago

Replying to azaozz:

It's not hard to check for a dot in the hook name or if the callback is a string.

Supporting string callbacks doesn't make sense in JavaScript - you should just pass the function directly instead. Functions also have a name property which we can check.

However for ease of use/clarity it would probably be better to always require a jQuery-style namespace.

Then we are back to this point, again for ease of use/clarity:

if we do make [the namespace] mandatory, then it should be a separate argument (at that point there's no reason to include it in the "hook name" argument and then have to split apart that string).

Possible solutions so far:

  • Always require a jQuery-style namespace.
  • Always require either a jQuery-style namespace or a function name.
  • Add a separate, required argument for hook name.
  • Decide that forEachFilters provides enough of this functionality for us to be OK with not having mandatory names.

I have to say that none of these really feel right to me. I will keep thinking about it; other suggestions welcome.

#138 in reply to: ↑ 137 ; follow-up: @adamsilverstein
4 months ago

Functions also have a name property which we can check.

right this would be a good way to check.

However for ease of use/clarity it would probably be better to always require a jQuery-style namespace.

I would also be happy with supporting namespacing and always using namespacing in core, in our examples and documentation, , JS standards & linting without actually enforcing that restriction in code.

#139 in reply to: ↑ 138 @azaozz
4 months ago

Replying to adamsilverstein:

I would also be happy with supporting namespacing and always using namespacing in core, in our examples and documentation, JS standards & linting without actually enforcing that restriction in code.

Think we are getting to a "weird" place here. That means we will have one standard for core and another for plugins. Why? Wouldn't that inconsistency be a bad thing? And at the end: what are we gaining by having a "double standard" :)

#140 in reply to: ↑ 137 @azaozz
4 months ago

Replying to jnylen0:

Then we are back to this point, again for ease of use/clarity:

if we do make [the namespace] mandatory, then it should be a separate argument (at that point there's no reason to include it in the "hook name" argument and then have to split apart that string).

Hm, think there is some misunderstanding here :) I don't have strong preferences to either way (see comment 131).

The facts are that:

  1. In the current implementation (PHP) all hooks are removable.
  2. There are some comments on this ticket suggesting to make some of the hooks non-removable in JS.
  3. I'm against that and think all hooks should also be removable in the JS implementation.

I have to say that none of these really feel right to me. I will keep thinking about it; other suggestions welcome.

Yeah, I tend to agree. IMHO best of the four options so far is adopting the jQuery-style namespaces as they are the most widely used/known. If there is a better way, I'm all for it.

#141 @azaozz
4 months ago

Related: #41422.

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


4 months ago

This ticket was mentioned in Slack in #core-js by jnylen. View the logs.


4 months ago

#144 @adamsilverstein
3 months ago

  • Keywords has-unit-tests added

In 21170.13.diff:

Bring the plugin code (and tests) up to date with the latest development work from https://github.com/WordPress/packages/pull/13, notably:

  • Add a new, required namespace parameter to addHook and removeHook functions.
  • Note, the readme is updated with the new function signatures.
  • removeHook functions now take a hookName and a nameapace, not a callback.
  • Consistent validation of hookName and namespace parameters.
  • Better support for recursion and generally calling and removing hooks inside other hooked callbacks (additional tests added for this in the package build).

#145 @adamsilverstein
3 months ago

In 21170.14.diff :

  • Update the namespace parameter to use the form vendorName/pluginName/functionName as per feedback from @schlessera.
  • Update tests to match.

#146 @adamsilverstein
3 months ago

  • Keywords commit added

#147 @adamsilverstein
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 41375:

Add wp.hooks - JavaScript actions and filters.

Add a JavaScript hooks library with an API that mirrors the WordPress Plugin API; provides similar functionality and API to PHP hooks.

Called via the global wp.hooks, eg: wp.hooks.addAction(), etc. Adds:

  • addAction( 'hook', 'vendor/plugin/function', callback, priority )
  • addFilter( 'hook', 'vendor/plugin/function', callback, priority )
  • removeAction( 'hook', 'vendor/plugin/function' )
  • removeFilter( 'hook', 'vendor/plugin/function' )
  • removeAllActions( 'hook' )
  • removeAllFilters( 'hook' )
  • doAction( 'hook', arg1, arg2, moreArgs, finalArg )
  • applyFilters( 'hook', content, arg1, arg2, moreArgs, finalArg )
  • doingAction( 'hook' )
  • doingFilter( 'hook' )
  • didAction( 'hook' )
  • didFilter( 'hook' )
  • hasAction( 'hook' )
  • hasFilter( 'hook' )

Props adamsilverstein, jnylen0, aduth, kadamwhite, youknowriad, schlessera, mikeschinkel, azaozz, vhauri, CaptainN, scribu, carldanley, chetanchauhan, mgibbs189, stephenharris, justnorris, koopersmith, gcorne, TV productions, atimmer.

Fixes #21170.

#148 follow-up: @pento
2 months ago

  • Keywords has-patch has-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Unfortunately, this was committed with a blocker not being addressed.

I only had a brief chance to bring this up in the PR, but there are several problems with this going into Core now.

Primarily, it's mostly unused in real situations (ACF being the notable exception), and entirely unused (to my knowledge) in a JS project using modern practices. While wp.hooks would potentially help with our legacy JS, new APIs need to be thinking about the future of WordPress' JS development. We're in a huge amount of flux at the moment, so focussing on new JS practices, then working on making those APIs available to legacy JS, gives us much more flexibility for the next 10 years of WordPress development. Locking ourselves into a thing built with WordPress' JS development of 5 years ago in mind is best case limiting our potential, worst case causing significant backwards compatibility issues that we'll need to deal with in 6-12 months.

Even not considering modern practices, wp.hooks has no usage in Core, some parts of Core's legacy JS should be making use of it when it's committed.

While the pub/sub model has worked really well for WordPress' PHP pluggable interfaces, and has been historically used with JavaScript's event model, it's not necessarily the way forward, and making it the "WordPress way" limits our options.

So, here's what I suggest:

  • Revert [41375] for now, with the explicit aim of committing wp.hooks in the future.
  • Publish wp.hooks on NPM as 1.0.0.
  • Try using the wp.hooks module in Gutenberg. What works? What doesn't?
  • Iterate.
  • If needed, release wp.hooks 2.0, 3.0, however many API-breaking releases we need.
  • Once the modern practices are settled, backport to work with legacy JS. Add it into some of Core's legacy JS, so we get practical usage there, too.
  • If we haven't already, have a better way of importing modules into Core. Committing the webpack-ed version of the module to the SVN repo is unsustainable.
  • Commit wp.hooks to Core.

This gives wp.hooks some usage based on modern practices, it gives the upheaval in WordPress' JS development practices some time to settle, and it gives us a chance to build something that will work in the modern JS world, and in the legacy world. Maybe that thing will look quite similar to what wp.hooks currently looks like, maybe it won't. But now is a particularly bad time to be making that bet.

#149 @Mte90
2 months ago

In my modest and little opinion this implementation is very cool and I don't think that can create issues on WordPress but yes implementing and not using inside the core is not very useful.
The hook system in WordPress is cool because is integrated in the core so the developers can alter the behaviour so have the API and not using is not very useful, because developers can implement on their own a similar api in case.

In any case the NPM package is interesting and probably is the case to evaluate where can be useful this API inside the core (also for Gutenberg). I think inside where there is an huge use of JavaScript like in the media picker, Tinymce, Widget Admin View etc.

#150 @flixos90
2 months ago

I'd also like to throw in another suggestion (which might be out of scope for the initial set of functions, but is worth considering IMO):

Due to a lot of processes in JS being asynchronous, I've been thinking about asynchronous hooks. The currently suggested API monitors pretty much how the API works in PHP, however there may be cases in JS where none of these functions work, such as the following:

  • Imagine there's a JS filter for some data.
  • Someone wants to use that filter and override that data with content from an AJAX call, like to the WP REST API.
  • This is currently not possible since the AJAX call is asynchronous while the filter is synchronous and expects an immediate result.

A possible approach to make something like the above work would be something like doActionAsync( 'hook', callback, arg1, arg2, moreArgs, finalArg ), where callback is the callback that should fire after all hooks attached to the action have been fired (should always be parameter-less). Internally, doActionAsync() would work like this:

  • If no hooks are attached, immediately call the callback.
  • Otherwise call the first hook and pass a callback to it which will call the next hook (the attached hook would be responsible for calling the callback after it has been completed).
  • If there is no more next hook, all attached hooks have been run and the original callback passed to doActionAsync() would be called to proceed with the original logic.

Note that the above approach basically waits for each hook's completion until calling the next one. An alternative approach could be to call all hooks immediately and only wait more generally until every one has completed until calling the original callback.

There could also be a applyFiltersAsync( 'hook', callback, content, arg1, arg2, moreArgs, finalArg ) which would work almost the same like the above, with the exception that the callback would always require one parameter, which should be the content. This would be passed on from attached hook to hook.

Due to the complexity of this ticket already it may be better to think about the above later and deal with it in a separate ticket after the foundation is set in stone. I just wanted to leave this here for now. :)

#151 @adamsilverstein
2 months ago

@pento I respectfully disagree with your conclusion here. As a note, I committed this code after careful discussion and several public comments affirming the commit by WordPress Lead developer @azaozz as well as feedback and contributions from some of core's most experienced JavaScript contributors.

Unfortunately, this was committed with a blocker not being addressed.

Your blocker seems contrived: we can't add a library to core until it is already used? Just last week, our project leader @matt was telling us to have more iteration happening in trunk. Having this in trunk means we can build off it, using it in core itself and we have plans to do just that.

Even not considering modern practices, wp.hooks has no usage in Core, some parts of Core's legacy JS should be making use of it when it's committed.

What? We can't add usage in core until hooks is in core. And indeed, now that it is in core, the plan is to move forward by adding extensibility to core JavaScript via wp.hooks. For example, one common need extending the media modal and wp.hooks would be perfect to add these capabilities we are discussing on #40427.

For example, I recently worked to help introduce the image widget to core. In this PR, you can see the (huge amount of) code needed to the get the 'Insert Media' button working (all we wanted was to remove the add gallery/playlist options). Having filters in place in wp.media would greatly simplify these types of customizations.

it's not necessarily the way forward, and making it the "WordPress way" limits our options.

I disagree. Sure, it may not be the way forward with Gutenberg for example, why does that limit our options? We have needed a way to provide more JS extensibility for 5+ years, and wp.hooks addresses that need. I haven't heard a final extensibility approach articulated by Gutenberg (or Calypso for that matter), and whatever they choose may not be applicable to our 35k lines of legacy JavaScript and Backbone code.

It seems like you are afraid to include hooks because it may lock us into something outdated. Please consider that this is a proven, well tested library that will let us make our existing JS more extensible. Sure - it may not work for our future needs - that shouldn't stop it from solving our current needs with a proven approach. Also, it is a tiny library that hasn't changed much in years, so the maintenance burden will be small.

Once the modern practices are settled, backport to work with legacy JS. Add it into some of Core's legacy JS, so we get practical usage there, too.

This is fanciful. We don't know what these practices will be or whether they will even work with our legacy JavaScript. Why are we discarding a proven approach we know we can use now for some imaginary approach that may arrive at some time in the future? Many smart people worked very hard on this library and getting in core for good reason - they want to use it in their development work. It seems like you are saying we should not add it to core because its not yet used or because some new approach may work better. This hinders our progress in core JavaScript instead of forwarding it.

If we haven't already, have a better way of importing modules into Core. Committing the webpack-ed version of the module to the SVN repo is unsustainable.

I agree. We are planning to include this from npm directly once its published, as part of our build process once we switch to webpack from browserify we will have what we need (#40894). This can still happen in 4.9.

I would be interested to hear more feedback from @azaozz or other lead developers, as well as from members of the Gutenberg team such as @aduth or @jnylen0. Does wp.hooks seem like something that would be genuinely useful for WordPress core and plugin/theme developers or in your project? Are you working on a new approach to extensibility that will be applicable to core that we should be waiting for? Also, @westonruter have you looked at wp.hooks, is it something you think would be useful in making the Customizer more extensible?

Last edited 2 months ago by adamsilverstein (previous) (diff)

#152 @adamsilverstein
2 months ago

@flixos90 Thanks for bringing up async support. We discussed this briefly on the PR and decided it was out of scope for 1.0.0 of wp.hooks. I would love to explore in a future version. One possible approach to explore for async with the current implementation for developers is to run a hook before the async action, and run another once the async action completed. Another option would be to return a promise from the filter.

#153 @dd32
2 months ago

To get right to the point, I have to mostly agree with @pento here - I don't think it's quite the right moment in time for this.

5 years ago, I think this would've been a great addition to the state of Javascript in WordPress - taking a concept WordPress developers were familiar with and providing a similar mechanism for the jumbled random pieces of Javascript inserted by all sorts of things, and actually getting it to all work together cohesively.
But it's not 5 years ago, it's now 2017 and WordPress is behind the curve on modern Javascript adoption, we're playing catch up and we need to be looking at what will be useful over the coming 10 years, and not just what has proved reliable in the past.

The good news is that I don't think this patch/approach is incorrect or bad at all - Hooks & filters have certainly proved to be reliable in PHP, I just don't want us to run forward into adopting something which is based on historical JS usage in WordPress rather than future JS usage.
I'd want to see something using a modern library (Be that React, Vue, Angular, Whatever) using this pattern before we add it as-is to WordPress, if it proves to be useful in that context then I'd be all for this, but only after a reasonably complex application had tried it.
Gutenberg is the current obvious choice and if using it in Gutenberg didn't show up some shortcomings of the API as-is I'd be extremely surprised.

Last edited 2 months ago by dd32 (previous) (diff)

#154 @lgedeon
2 months ago

I am going to jump out on the limb and cut it right off. One of the great things about WordPress is that it is not on the cutting edge.

You can use the same old code year after year and it still works. Sure we add new features when we can but if the old way isn't blocking a feature why not keep it around?

Hooks have served us well for years, and can keep working for another ten years. By then maybe we will have an all new WordPress that is 100% JS or some language that is even better. Then we can write a migration script to move everything to the new system. As long as the UI is similar to what writers are used to and gets as much attention to excellence as the current UI that will be fair play. But, I digress.

For right now, this ticket is the best solution. Maybe a year from now we will come up with an idea we like better, but then we will spend five years getting it ready and this solution will have kept us going all that time.

Maybe part of the api will need to change and ten plugins will need to refactor, but that is better than the current state where we are having to constantly work around stuff or fork a whole file just to get something to work.

As a nearly perfect perfectionist, this is hard for me to say, but it is time to just ship this thing.

"Give them the third best to go on with; the second best comes too late, the best never comes."

#155 @matveb
2 months ago

[...] as well as from members of the Gutenberg team [...] Are you working on a new approach to extensibility that will be applicable to core that we should be waiting for?

We have been experimenting with decorators, exploring slot & fill patterns, and dabbled with middlewares. There are some great points in these to build from. We do need some more tangible examples to define and implement any solution. Gutenberg gives us a good place to try them.

#156 @adamsilverstein
2 months ago

We have been experimenting with decorators, exploring slot & fill patterns, and dabbled with middlewares. There are some great points in these to build from. We do need some more tangible examples to define and implement any solution. Gutenberg gives us a good place to try them.

@matveb do you feel that decorators, slot and fill patterns or middleware would be applicable to extending our existing core JavaScript, especially our single page Backbone Apps?

I'd want to see something using a modern library (Be that React, Vue, Angular, Whatever) using this pattern before we add it as-is to WordPress, if it proves to be useful in that context then I'd be all for this, but only after a reasonably complex application had tried it.

@dd32 I'm happy to see Gutenberg exploring wp.hooks as a possible api for adding extensibility. At the same time, I am trying to forward the ongoing effort to make our existing JavaScript more accessible and extendable - especially our single page apps - the media modal, the media grid, the new theme browser, the installed theme browser, the customizer, the editor and the revisions screen (i may have missed some). wp.hooks seems like a great fit make these components more extendable, 'modern' JavaScript approaches may not be as good a fit. How would you propose we work on making our existing JavaScript more extendable in core? Wait until the Gutenberg project decides how to approach extendability, then see if we can retroactively apply that approach to our legacy JavaScript?

Last edited 2 months ago by adamsilverstein (previous) (diff)

#157 @dd32
2 months ago

How would you propose we work on making our existing JavaScript more extendable in core?

Honestly, I can't answer that right now. However a lot of the code in question has been operating in it's current state for quite some time, another few {insert time length here} isn't going to do a huge amount of harm in the long run. Many have some form of extensibility worked in through unexpected and sometimes convoluted methods that others have already been using.

I'm more concerned with having building blocks for the future that feel native, rather than potentially forced to fit in. In other words, I'd rather attempt to retroactively apply whatever the ultimate solution here to legacy code, than for future code to build upon something that was designed for a different era of Javascript. If that means that some legacy code remains non-accessible/extendable even longer, I'm not too worried about that.

Whether we like it or not, a lot of the existing Backbone-based interfaces are likely to change in the future as design/feel/JS code/etc is integrated into a cohesive platform that works the same, no matter which page you're looking at.
Once upon a time it looked like all of WordPress would be built upon Backbone, today, finding someone who would choose Backbone over anything else is near impossible. I would not be surprised if all of the Backbone apps you listed were not rewritten (or being rewritten) using a react-like library within the next 3 years (to throw some form of timeline out there, that I'm fairly confident of).

#158 @azaozz
2 months ago

I tend to agree with @dd32. @matveb puts it quite nicely:

We do need some more tangible examples to define and implement any solution.

Think it will be good to "proof-test" this in Gutenberg and change/replace it if it doesn't meet the expectations or doesn't fulfill its purpose.

I mean it works quite nicely with the current JS in core, but if it is going to be unusable in the "future JS", better to either make it work there or change to something that will work.

Last edited 2 months ago by azaozz (previous) (diff)

#159 @adamsilverstein
2 months ago

Ok @azaozz, thanks for weighing in although this is a big change since you told me I should commit this code a few days ago.

I regret having invested so much time and effort over the last two years working to bring this to core, and wish these concerns and "blockers" had been raised much, much earlier and not at the last minute.

We should be collaborating as a team on these efforts, not arguing the merits of a library after a commit with demands for a revert.

#160 @adamsilverstein
2 months ago

  • Owner adamsilverstein deleted
  • Status changed from reopened to assigned

#161 @adamsilverstein
2 months ago

I would not be surprised if all of the Backbone apps you listed were not rewritten (or being rewritten) using a react-like library within the next 3 years

Including the Customizer? So should we stop working on improving these JavaScript components?

#162 follow-up: @adamsilverstein
2 months ago

We do need some more tangible examples to define and implement any solution.

@matveb @azaozz @pento @dd32

Here are some tangible examples of wp.hooks already being used in a popular plugins:

wp.hooks is included in Shortcake. This plugin has over 20k active installs and is actively used on very high traffic enterprise level sites.

wp.hooks is used in Advanced Custom Fields. This plugin has over 1 million active installs.

Primarily, it's mostly unused in real situations

False. It is actually used in production on large sites in real situations.

I'd want to see something using a modern library (Be that React, Vue, Angular, Whatever) using this pattern before we add it as-is to WordPress

Why? we don't make use of any 'modern library' in core today and this approach may not be applicable to modern JS anyway. Using this approach on our current JS in no way precludes using a different approach in the future which is I think the core of our disagreement here. Why do you assume using wp.hooks for legacy extensibility means we are locked into using it for Gutenberg?

Choosing what we use in core today based on the needs of Gutenberg is a mistake. The decorators, slot & fill patterns, and middlewares being explored there are React/Redux based approaches that are unlikely to be helpful for extending legacy JavaScript.

Last edited 2 months ago by adamsilverstein (previous) (diff)

#163 @mgibbs189
2 months ago

This revert is unsettling on so many levels.

5 years ago, I think this would've been a great addition to the state of Javascript in WordPress

The patch was polished and merge-ready for 3.7... as in 2013. Lots of great contributors like @adamsilverstein @mikeschinkel and @carldanley poured their expertise into this. wp.hooks is just as relevant today as it was 5 years ago.

I'm more concerned with having building blocks for the future that feel native, rather than potentially forced to fit in.

wp.hooks is just a helper library (like lodash) -- only w/ methods to prioritize JS execution. The simple inclusion of this library will NOT interfere with whatever framework WP chooses.

Also, what exactly about wp.hooks is "legacy"? Just the fact that it's been around a while? The notion that "there may be a better approach in the future" is NOT a good reason to keep kicking the can, especially since (A) there's no specifics about an alternative, and (B) there's been a need for this utility for years.

Using this approach on our current JS in no way precludes using a different approach in the future

Exactly!

#164 follow-up: @sudar
2 months ago

I have been following this ticket for years. I got excited when it was finally merged it, I even tweeted about it. But when I woke up the next day it was reverted and I was very sad. I felt like a kid who’s birthday gift got snatched before they could use it.

I am just an ordinary developer who works on mostly plugins and this would be extremely valuable for me personally. All these years there have been numerous occasions when I could have made use of it, if it was available in core.

The event based approached (actions & filters) in PHP has worked really well for WordPress and most developers who work in WordPress understand it. If it is extended to JavaScript it would benefit lot of (ordinary) developers like me. If I remember correctly WordPress has always preferred to keep the barrier to entry low and make it easy for developers to understand the code instead of just chasing the latest and greatest. (We still haven't used namespaces in PHP)

Also, what exactly about wp.hooks is "legacy"? Just the fact that it's been around a while? The notion that "there may be a better approach in the future" is NOT a good reason to keep kicking the can, especially since (A) there's no specifics about an alternative, and (B) there's been a need for this utility for years.

Exactly!

Choosing what we use in core today based on the needs of Gutenberg is a mistake. The decorators, slot & fill patterns, and middlewares being explored there are React/Redux based approaches that are unlikely to be helpful for extending legacy JavaScript.

Denying developers an easy to use and simple to understand library that they could use today (could have used all these years if it was merged) on the basis that something that _might_ get merged in the future (who knows how long it is going to take) may not need it is a very bad mistake, especially when it is not mutually exclusive.

#165 in reply to: ↑ 162 @Mte90
2 months ago

Here are some tangible examples of wp.hooks already being used in a popular plugins:

I was following this ticket because in the past I made something like that for a my plugin premium never released and abandoned so is not public. I was using a custom system made by me to enable to create other plugins that can improve the javascript that my plugin was implementing.
If it is available in the core something like that I don't know if my plugin would be published but I know that probably I should lost time to doing something else.

I tweeted to and share on the net about this new implementation because save times, is a standard and is available and important will be used from the many integration in browser.
Actually for me doesn't matter if something is done with backbone or react (I don't like react personally for the license issue as example) but I know that I would need probably this API in the future and as developer I want that stuff and not wait or say hello to an amazing integration.

When I have to talk to my customers I cannot say the implementation will be in 2 year, they want the stuff right now, and I have to do it on my own. So like with the rest API released piece by piece inside WordPress I think that is possible also for this one as already explained.

Of course if it will be used inside the core or make available to developers inside the core will be so very cool :-)

Last edited 2 months ago by Mte90 (previous) (diff)

#166 @maguiar
2 months ago

Choosing what we use in core today based on the needs of Gutenberg is a mistake. The decorators, slot & fill patterns, and middlewares being explored there are React/Redux based approaches that are unlikely to be helpful for extending legacy JavaScript.

I agree so much with this, besides, is that the path we are taking? If it doesn't work with Gutenberg it just doesn't work anymore?

#167 @jadpm
2 months ago

Just wanted to stop and say that I am also following this ticket for years now (the number of people subscribed here may give an idea of whether this is really an interesting project or what), and I was really shocked to read that a revert on this merge was even proposed for no real reason besides that maybe someday somehow someone would propose a better (?) approach.

I might be entirely wrong, but the approach of this ticket was basically extending the successfull hooks mechanism from PHP to JS, so I would be extremely excited and maybe a little surprised to see an alternative approach that does just that.

Finally, I am the lead developer of a set of commercial plugins which has been using a ported namespaced clone of this library for some months now, and not only we found it valuable and usefull but it proved itself as a reliable extension of the plugins API that made WordPress so easy to use for developers.

I personally would like to reserve my opinion about Gutenberg since neither this is the ticket to discuss it, nor it is the measure of all WordPress things happening now. Hope everybody could understand that too.

#168 @verticalgrain
2 months ago

I wanted to chime in and say I was excited to see this ticket merged and quite disappointed to see it being reverted.

I hope that the reaction of so many developers who want this causes the decision to be reconsidered.

#169 in reply to: ↑ 164 @MikeSchinkel
2 months ago

Replying to sudar:

I have been following this ticket for years. I got excited when it was finally merged it, I even tweeted about it. But when I woke up the next day it was reverted and I was very sad. I felt like a kid who’s birthday gift got snatched before they could use it.

Exactly how I felt.

To date it has been very hard to extend code in WordPress written in Javascript. wp.hook finally offered a solution, and it has been far too long in coming.

I really hope that "perfect" is not allowed to exterminate "good" here.

#170 @pbearne
2 months ago

I was so looking forward to this

Please revert the revert :-)

Paul

#171 @matveb
2 months ago

Choosing what we use in core today based on the needs of Gutenberg is a mistake.

Sorry, I didn't mean to imply that. I think what @azaozz means is that we have a nice opportunity to immediately test the approach in a large-ish JS project for core as part of one of the focuses—which might tell us if it's at all viable or desirable for it, illustrate inherent problems/flaws/opportunities we would want to address, or just learn that it's orthogonal to whatever approach we take for Gutenberg and it's fine to separate extensibility of current JS code from newer.

#172 @azaozz
2 months ago

Yep, exactly. Sorry if that was misunderstood. I didn't mean Gutenberg as the new editor, I meant it as a good way to test and experiment with wp-hooks in a forward-looking way.

#173 @adamsilverstein
2 months ago

Thanks for the clarification. I am curious to see if wp.hooks proves useful in Gutenberg, or what changes that use might bring up! In the meantime, I would also like to start using wp.hooks in core.

The goal of getting wp.hooks into trunk is to start trying to use is internally - I'm going to work on a demonstration of that next.

If we find the wp.hooks library is not useful or feel its API is not stable before the 4.9 release, I would support reverting or possubly authoring a dev note as well as liberal inline comments explaining that this is a new API that may have breaking changes and if developers use it they should watch development closely.

Last edited 2 months ago by adamsilverstein (previous) (diff)

#174 follow-up: @PeterRKnight
2 months ago

Regardless of the sudden change of course, will we see the hooks API on npm soon? It would be good to use in plugins right now.

Although it seems reasonable to hold off on this, it is striking how reluctant we are to embrace things that have served WordPress well as we attempt to modernise. Hooks & filters are the bread and butter of extending WordPress, it's one of the most battle tested overwhelmingly successful design patterns but it seems like there's a negative bias against it simply because it is not new? Hooks and filters are something that WordPress has gotten right despite all the flack WordPress gets for its architectural state.

I'm not sure there is a JS project out there that has achieved the same ease of customization, allowing 3rd parties to modify just about every behaviour and string of an app on the same scale as we find in WordPress. So I don't understand why we've become transfixed with JS frameworks and evolving practices found in projects that don't even begin to resemble or emulate the needs of WordPress. They're not designed for an ecosystem of thousands of plugin and theme developers that want the power to change every piece of behaviour. Instead it seems like we need the blessing of some mythical standard in the JS developer community rather than forging our own decisions based on what we know works, and perhaps even doing things unique to WordPress.

Last edited 2 months ago by PeterRKnight (previous) (diff)

#175 in reply to: ↑ 148 @westonruter
2 months ago

Replying to pento:

  • If we haven't already, have a better way of importing modules into Core. Committing the webpack-ed version of the module to the SVN repo is unsustainable.

This is a need for the newly-committed CodeMirror bundle as well. See #41870 (Code Editor: Add grunt task for building new CodeMirror bundles from external dependencies)

Replying to adamsilverstein:

Also, @westonruter have you looked at wp.hooks, is it something you think would be useful in making the Customizer more extensible?

The Customizer has it's own events interface which has served it well. I suppose I'm so accustomed to to the way that JS classes in the Customizer have been extended with subclasses and function wrapping that I haven't felt the need for hooks. The main problem with the current system for extending Customizer JS (and in Media JS) is that it is not particularly elegant to be interfacing with function prototypes all over the place, IMHO. I'd have to do an analysis of all of the Customizer feature plugins to see all of the ways that it is being extended. The normal way that I've implemented filter-like functionality in the Customizer is to wrap a wp.customize.Value's validate method to override its set value to always be as desired. For example, in Customizer Responsive Server-Side Components Device Preview it will force the URL in the wp.customize.previewer.previewUrl to always include the current previewedDevice value in a customize_previewed_device query parameter (link):

var originalPreviewUrlValidate;
originalPreviewUrlValidate = component.api.previewer.previewUrl.validate;
component.api.previewer.previewUrl.validate = function validatePreviewUrl( newUrl ) {
        var url = newUrl;
        if ( null !== url ) {
                url = component.amendUrlWithPreviewedDevice( url );
        }
        return originalPreviewUrlValidate.call( this, url );
};

It works very well, but it's not particularly elegant. It would be cleaner if there was some 'previewUrl' filter that could be used instead of having to wrap the validate function.

I'll note that I've tried to take a similar approach for filtering values being set on Backbone models, but there is no equivalent validate method in Backbone. The validate method on wp.customize.Value in the Customizer is really more similar to a sanitize method, whereas in Backbone it's used only as a strict a pass/fail kind of thing (like validate in the REST API). So in the media widgets, in order to implement the same kind of sanitization logic it is currently overridding the entire Model#set method itself.

One point of concern I have with wp.hooks as to me it would seem to encourage a break in the encapsulation of logic inside of individual components. In other words, JS components that would be running the hooks would all have to reach out to this one object in global scope where all of the hooks are registered. Namespaces would have to be added to the hooks to prevent them from conflicting due to the fact that they are all global.

This just came to mind now (and maybe this has already been considered in the design of wp.hooks, so excuse my ignorance): ¿would it not be a better architectural pattern to instead introduce hooks as something that can be instantiated onto a specific object? For example, Backbone has its Events mixin and Customizer similarly has its wp.customize.Events interface. You can augment any object with this mixin to get the interface to trigger and listen for events, and when the events are triggered you don't need a namespace because they're naturally scoped to that object. There is also less overhead then by not having to manage a massive object of hook/handler mappings with the risk then of leaking memory (I recall PHP actions and filters being identified as a performance bottleneck in WordPress).

In contrast, what if you have a wp.Hooks interface that you add to some Thing (like Object.assign( Thing.prototype, wp.Hooks ) and then you do obj = new Thing() and attach a filter to it like obj.addFilter( 'foo', function() {…} ) then when obj is destroyed then that anonymous function handler could also be garbage collected. In contrast, if you do wp.hooks.addFilter( 'someNamespace.thing.foo', function() {} ) the you'd potentially need to remember to removeFilter later or else that anonymous function will stick around forever.

Replying to adamsilverstein:

The goal of getting wp.hooks into trunk is to start trying to use is internally - I'm going to work on a demonstration of that next.

If we find the wp.hooks library is not useful or feel its API is not stable before the 4.9 release, I would support reverting or possubly authoring a dev note as well as liberal inline comments explaining that this is a new API that may have breaking changes and if developers use it they should watch development closely.

We're planning to be working on a Customizer v2 feature plugin after the 4.9 release, and at that point we'll definitely taking a look at the current JS API and anywhere that its extensibility is lacking. I'd want to incorporate wp.hooks wherever possible in that project.

In conclusion, I would be open to keeping wp.hooks in trunk for the remainder of 4.9-alpha with an expectation that it will be reverted before beta, if that helps with testing patches that prototype making use of wp.hooks. (While an exception was made for the REST API, I think we shouldn't ship something that isn't used in core.) In that way, it would be treated like “temp hooks” for feature plugins (which aren't used very often). But if having wp.hooks in core is only to facilitate writing patches for implementing hooks for parts of core, then I'd suggest instead creating a new “wp-js-hooks” feature plugin that simply bundles the JS and registers it so that other feature plugins can enqueue it and utilize it. (Plugin dependencies, anyone?)

#176 in reply to: ↑ 174 @netweb
2 months ago

Replying to PeterRKnight:

Regardless of the sudden change of course, will we see the hooks API on npm soon? It would be good to use in plugins right now.

Yes, it will land at https://www.npmjs.com/package/@wordpress/hooks very soon, days, not weeks :)

#177 follow-up: @adamsilverstein
2 months ago

media-button-filter-example.diff is an example use case for hooks in core. Note the red color of the patch as it simplifies what we have to do here... For the media widgets, when you click to insert media, we want it to read 'Insert into Widget' not 'Insert into Post'. Now, instead of having to overwrite the entire mainInsertToolbar function, we apply a filter which we can then use to alter the value on the fly.

would it not be a better architectural pattern to instead introduce hooks as something that can be instantiated onto a specific object

@westonruter intriguing idea, i'm not sure it lets you remove hooks the way we want? The namespaces is mostly so you CAN easily remove hooks by name (no matter who added them), a requirement that was brought up during development.

#178 in reply to: ↑ 177 ; follow-up: @westonruter
2 months ago

Replying to adamsilverstein:

would it not be a better architectural pattern to instead introduce hooks as something that can be instantiated onto a specific object

@westonruter intriguing idea, i'm not sure it lets you remove hooks the way we want? The namespaces is mostly so you CAN easily remove hooks by name (no matter who added them), a requirement that was brought up during development.

Sure, you'd just remove the hooks on the object that you added wp.Hooks to begin with. There would be no need for a hook namespace because each object that extends wp.Hooks would have its own set of hooks. For example, given:

var component = {};
_.extend( component, wp.Hooks );
var fooFilter = function( x ){ 
    return x + ' foo\'d!'; 
};
component.addFilter( 'foo', fooFilter );

Then that foo filter can be removed with either:

component.removeFilter( 'foo', fooFilter );

Or if the reference to fooFilter is not available then this can be used instead:

component.removeAllFilters( 'foo' );

#179 in reply to: ↑ 178 ; follow-up: @adamsilverstein
2 months ago

Or if the reference to fooFilter is not available then this can be used instead:

component.removeAllFilters( 'foo' );

@westonruter Right, this highlights an issue: if anonymous callbacks are added to hooks, we can't individually remove them and instead have to remove all the hooked callbacks. By adding the namespace requirement, we ensure the individual callbacks can always be removed (discussed somewhere above).

Regardless, this could still work as you are proposed - something instantiated on an object, that would simplify the namespacing, since the package/plugin part would be the object itself.

What other advantages do you imagine object extension vs the current approach would offer?

#180 in reply to: ↑ 179 ; follow-up: @westonruter
2 months ago

Replying to adamsilverstein:

What other advantages do you imagine object extension vs the current approach would offer?

The main benefits I see of a wp.Hooks mixin over a wp.hooks global are:

  1. Hooks are registered and deregistered along with the component they are modifying, reducing the amount of hooks in a global registry and eliminating the need to namespace hooks at all.
  2. Extending objects with an wp.Hooks interface is very much in line with out JavaScript projects add eventing, such as as event-emitter and Backbone Events and even wp.customize.Events in the Customizer for that matter. Alternatively to using object extending, this would work just as well with composition so you could do something like component.hooks = new wp.Hooks() and have component.hooks.addFilter(…) as opposed to _.extend( component, wp.Hooks ) and component.addFilter(…). Having a single global wp.hooks now feels like an anti-pattern in the age of packages and modules, whereas including an instance of wp.Hooks in each component feels very much in-line with standard practice in JS.
  3. There may be some performance benefits to reducing the size of the hook/handler hash, since there would be fewer entries to look for if the hooks are encapsulated in a specific object.
  4. Also, as I said before when hooks are encapsulated then there are more opportunities for automatic hook handler garbage collection since there would at least be an opportunity for an object extended with wp.Hooks to go out of scope, whereas a global wp.hooks would never go out of scope.
  5. Also, when registering hooks on objects instead of globally, there is the added benefit that when hooks are run, that the object itself could be supplied as this context for each handler. That could be handy.

#181 in reply to: ↑ 180 ; follow-up: @adamsilverstein
8 weeks ago

Replying to westonruter:

Excellent feedback @westonruter, thanks! I'm going to give this a try in a branch in the packages repo.

Having a single global wp.hooks now feels like an anti-pattern in the age of packages and modules, whereas including an instance of wp.Hooks in each component feels very much in-line with standard practice in JS.

Great point.

#182 follow-up: @mgibbs189
8 weeks ago

Having a single global wp.hooks now feels like an anti-pattern in the age of packages and modules, whereas including an instance of wp.Hooks in each component feels very much in-line with standard practice in JS.

What if one plugin needs to modify or remove hooks defined from another plugin? Or remove all handlers attached to a hook name? How would that look w/ the component-based approach?

@westonruter there's definitely pro's of component-based... but I still think the need remains for a wp.hooks global store as the single source of truth -- regardless of whether it feels like an anti-pattern.

#183 in reply to: ↑ 181 @azaozz
8 weeks ago

Replying to adamsilverstein:

Having a single global wp.hooks now feels like an anti-pattern in the age of packages and modules, whereas including an instance of wp.Hooks in each component feels very much in-line with standard practice in JS.

I actually think this is the opposite. The greatest strength of wp.hooks is that it is "above" any/all modules, packages, components, libraries, etc. and enables easy interoperability for all of them. Each library has its own native signalling methods that usually fit internal use best. We can, of course, enable wp.hooks to be used internally in modules, but thinking this will diminish its best user case: make it possible to use any library at any time with all plugins.

I do realize this is somewhat controversial, but then we are solving a problem the great majority of JS projects will never encounter: how to facilitate interoperability of several external components at the same time. Even the browser's events API is only targeted at single JS "source" and is not adequate when several different libraries try to cooperate and have to use the same events.

Last edited 8 weeks ago by azaozz (previous) (diff)

#184 in reply to: ↑ 182 ; follow-up: @westonruter
8 weeks ago

Replying to mgibbs189:

What if one plugin needs to modify or remove hooks defined from another plugin? Or remove all handlers attached to a hook name? How would that look w/ the component-based approach?

A plugin would simply have to call someOtherComponent.removeFilters( 'foo' ) instead of wp.hooks.removeFilters( 'someOtherComponentNamespace.foo' ).

#185 in reply to: ↑ 184 @adamsilverstein
8 weeks ago

@weston - I'm still intrigued by your suggested approach. I created a PR to explore this approach in the WordPress packages repository, I would appreciate your review and feedback there: https://github.com/WordPress/packages/pull/40

I can see some potential drawbacks to this approach, however this does help hooks bridge the gap into modern JavaScript and I could see this making hooks more generally useful. And as I mention in the PR, we could decide we still want wp.hooks in core even with this approach.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 weeks ago

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


7 weeks ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 weeks ago

#189 @adamsilverstein
7 weeks ago

After careful consideration around the risks involved in committing to this API as is, I am going to revert wp.hooks for the remainder of the 4.9 cycle with a plan to commit again early in the next release cycle. We released a beta of the hooks library on npm, so developers can (please!) try hooks now in JavaScript projects with npm install @wordpress/hooks and also follow development on the packages repository.

As disappointed as it is to see this reverted, I am excited about exploring adding hooks via composition which will make hooks more useful for legacy as well as modern JavaScript. This is why I have decided to revert for now: it is important we get the hooks library working just how we want it before we ship it with a WordPress release.

@adamsilverstein
7 weeks ago

revert commit

#190 @adamsilverstein
7 weeks ago

In [41751]:

Revert "Add wp.hooks - JavaScript actions and filters."
Revert wp.hooks for now as we continue to refine and test.
Reverts [41375].

#191 @jbpaul17
7 weeks ago

  • Milestone changed from 4.9 to Future Release

Punting this to Future Release per the @adamsilverstein's comments above.

This ticket was mentioned in Slack in #core-js by mte90. View the logs.


5 weeks ago

#193 @gziolo
43 hours ago

Where are we at the moment with hooks integration with WP core? 4.9 is out so it would be great to reconsider how the final API is going to look like. We are exploring how this could be consumed in Gutenberg. Related PR can be found here: https://github.com/WordPress/gutenberg/pull/3493. It looks like the version we merged and reverted a while back was pretty solid. At least this is what we consider at the moment to provide new ways to make Gutenberg extensible.

#194 follow-up: @adamsilverstein
15 hours ago

@gziolo Thanks for your work on this, that PR looks great and I will try to test it out.

Since this merge/revert here the code has only changed slightly, in this PR we changed the way hooks can be added to a project via composition (from npm): https://github.com/WordPress/packages/pull/40.

The hooks package now exports a createHooks helper:

Hooks can thus be added to an object via composition:

import createHooks from '../';
myObject.hooks = createHooks();
API functions are then be called: myObject.hooks.addAction()

The current plan for core is to experiment with adding hooks for extensibility of media and other JavaScript components and decide how to expose them. For a platform like WordPress core, it probably makes the most sense to expose hooks as a global as the previous patch did, eg. wp.hooks = createHooks(); while plugins may choose to their own namespace hooks, eg. myPlugin.hooks = createHooks();

#195 @adamsilverstein
15 hours ago

  • Milestone changed from Future Release to 5.0
  • Owner set to adamsilverstein

#196 in reply to: ↑ 194 @gziolo
15 hours ago

Replying to adamsilverstein:

The current plan for core is to experiment with adding hooks for extensibility of media and other JavaScript components and decide how to expose them. For a platform like WordPress core, it probably makes the most sense to expose hooks as a global as the previous patch did, eg. wp.hooks = createHooks(); while plugins may choose to their own namespace hooks, eg. myPlugin.hooks = createHooks();

Taking into account what you shared above and in the corresponding PR, I updated code to provide a compatibility layer for Gutenberg which exposes hooks as wp.hooks = createHooks(); to use globally for plugin developers whenever they want to update something in the editor. To modify experience around block we propose wp.blocks.addFilter, etc to narrow down the scope and provide consistent API with wp.blocks.registerBlock. We can discuss what is currently propose at the upcoming JS Core chat.

Note: See TracTickets for help on using tickets.