Skip to content
This repository has been archived by the owner. It is now read-only.

Add the WordPress hook library from 21170-core #13

Merged
merged 98 commits into from Sep 22, 2017

Conversation

Projects
None yet
@adamsilverstein
Copy link
Collaborator

commented Jul 18, 2017

@adamsilverstein adamsilverstein changed the title Add the wordpress hook library from https://core.trac.wordpress.org/ticket/21170 Add the WordPress hook library from https://core.trac.wordpress.org/ticket/21170 Jul 18, 2017

@nylen
Copy link
Member

left a comment

The machinery around createSomeFunction( type ) and all the corresponding HOOKS[ type ] checks feels a bit overwrought (and will perform more slowly than necessary). How about something like this instead:

const HOOKS = {
    actions: {},
    filters: {},
};

function createAddHookFor( hooksOfThisType ) {
    // Manipulate `hooksOfThisType` directly
}

export addAction = createAddHookFor( HOOKS.actions );
@@ -0,0 +1,18 @@
{
"name": "@wordpress/hooks",
"version": "1.0.0",

This comment has been minimized.

Copy link
@nylen

nylen Jul 19, 2017

Member

Let's make this 0.1.0 or something, I don't think we're at "final" v1 yet.

This comment has been minimized.

Copy link
@adamsilverstein

adamsilverstein Jul 19, 2017

Author Collaborator

Ok!

/**
* Internal Dependencies
*/
import { doAction, applyFilters, addAction, addFilter, doingAction, doingFilter, didAction, didFilter, hasAction, hasFilter, removeAllActions, removeAllFilters, currentFilter } from '../';

This comment has been minimized.

Copy link
@nylen

nylen Jul 19, 2017

Member

Let's split this long line up into one import per line.

This comment has been minimized.

Copy link
@rmccue

rmccue Jul 19, 2017

Collaborator

We could also export default { doAction, ... } and then just import hooks from '../, would likely be a bit nicer.

This comment has been minimized.

Copy link
@nylen

nylen Jul 19, 2017

Member

The drawback there is that you have to prefix every function call with hooks.

This comment has been minimized.

Copy link
@adamsilverstein

adamsilverstein Jul 19, 2017

Author Collaborator

I'll work on cleaning this up and maybe splitting out the generator code into files/export/import.


// Doing functions.
export doingAction = createDoingHookByType( 'actions' ), /* True for actions until;next action fired. */
export doingFilter = createDoingHookByType( 'filters' ), /* True for filters while;filter is being applied. */

This comment has been minimized.

Copy link
@nylen

nylen Jul 19, 2017

Member

Why the semicolons in the middle of these comments? Should be // inline comments as well

This comment has been minimized.

Copy link
@adamsilverstein

adamsilverstein Jul 19, 2017

Author Collaborator

thanks for noting, will correct

*/
function createRemoveAllByType( type ) {
return function( action, type ) {

This comment has been minimized.

Copy link
@nylen

nylen Jul 19, 2017

Member

No implementation yet?

This comment has been minimized.

Copy link
@adamsilverstein

adamsilverstein Jul 19, 2017

Author Collaborator

fixed on the ticket, will update...

@nylen nylen changed the title Add the WordPress hook library from https://core.trac.wordpress.org/ticket/21170 Add the WordPress hook library from 21170-core Jul 19, 2017

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2017

The machinery around createSomeFunction( type ) and all the corresponding HOOKS[ type ] checks feels a bit overwrought (and will perform more slowly than necessary).

@nylen I'm not sure I understand how exactly you are proposing refactoring this and how it would help... the type part is a wrapper around the generators to account for our two hook types.

@nylen

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

@nylen I'm not sure I understand how exactly you are proposing refactoring this and how it would help... the type part is a wrapper around the generators to account for our two hook types.

Instead of passing around type strings, pass around the actual objects that need to be manipulated instead (HOOKS.actions and HOOKS.filters in the current implementation).

This should make the code clearer, shorter, and faster.

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2017

Instead of passing around type strings, pass around the actual objects that need to be manipulated instead (HOOKS.actions and HOOKS.filters in the current implementation).

Ah, ok - interesting idea, I can see how that would be cleaner and faster. I'll give it a try on the core patch where I have unit tests that will tell me if I broke anything.

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2017

@nylen

This comment has been minimized.

Copy link
Member

commented Jul 20, 2017

Yes, except several operations are O(total number of hooks) and this should be O(total number of hooks with the given name) instead. Let's move development including unit tests to here - I can help with that tomorrow.

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 20, 2017

@nylen

several operations are O(total number of hooks) and this should be O(total number of hooks with the given name) instead.

Not sure what you mean by this, can you clarify?

I reworked the tests as Jest tests.

Thinking of breaking out the code into files and maybe temporarily adding a build step to generate the core file so I don't have to keep updating both.

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 20, 2017

I broke out the code a bit and tested the built in build process. What is the best way to get a build step that sets up a single file we can load into core that loads functions on wp.hooks? Do we need webpack with an entry file like this? https://gist.github.com/adamsilverstein/4fbbd4ca14c5f7526a1ee41b63509cd2

@nylen

This comment has been minimized.

Copy link
Member

commented Jul 20, 2017

Webpack can do that part for us, like the Gutenberg build does: https://github.com/WordPress/gutenberg/blob/c78ad83/webpack.config.js#L49-L74

That should probably be handled at the root level of this repo, though also related is how we'll publish the package on npm.

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 20, 2017

Ok, i'll work on the build part in the root with webpack, can we publish the package as @wordpress/hooks - and do we let people import individual functions, or only the entire hooks object?

@nylen

This comment has been minimized.

Copy link
Member

commented Jul 20, 2017

People should be able to import individual functions.

Centralize all run logic into `createRunHook`
Also fixes a bug that `doingAction` would remain true after its action
was completed.  Effectively it would always be true.

@nylen nylen force-pushed the adamsilverstein:add-wp-hooks branch from 2686c29 to 57f6038 Jul 20, 2017

@nylen nylen force-pushed the adamsilverstein:add-wp-hooks branch from e209b93 to 1901b43 Jul 20, 2017

@schlessera

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

As discussed in Slack, I'd like to suggest avoiding potential conflicts by changing the namespace prefix from <package>/<function> to <vendor>/<package>/<something>.

The reasoning behind this is that, for similar solutions, people tend to pick similar names. This makes the most important namespace level (<package> in this case) very prone to conflicts.

Adding the vendor prefix in front of that is a best practice in use in many different languages/frameworks, amongst others in the larger PHP ecosystem, the .NET ecosystem or even GitHub repositories like this one: WordPress\packages.

For the example of the WP Core hooks above, you'd have wordpress\core\something instead. This opens the way for wordpress\p2\something or any other official package that is part of the WordPress vendor namespace.

I do understand that you want to keep the strings as short as possible, but maybe it would make sense to provide an automated/more convenient mechanism for creating unique hook name instead in this case.

You could then have 1 central location where one sets up the namespace to use. People that need more control can still do it the more traditional way.

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 17, 2017

@schlessera - I updated the namespace as per your suggestion, thanks!

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 4, 2017

@nylen @kadamwhite @youknowriad @aduth - I'm planning to merge this to WordPress core soon so it has some time to soak for 4.9. Any final feedback here?

Can we go ahead and merge this and release as a package version 1.0.0?

@aduth

This comment has been minimized.

Copy link
Member

commented Sep 4, 2017

I worry this was driven by a desire to mimic PHP, "what if x"s and "maybe it'd be nice if x" than it was by real-world requirements, ease-of-use, or expectations via prior art in JavaScript, culminating in awkward semantics around vendoring, namespaces, and hook names. If you count each of these as a pseudo-argument of sorts, it requires a minimum of 5 arguments to hook anything. I think hook naming was a valid direction to explore, but not one that ultimately had a positive outcome.

In place of naming to remove hooks, I'd rather we had taken the direction of some precedent:

Or even a combination of the first and second.

Where are the requirements that dictated we should have addressable names for hooks? Even if we wanted to allow plugins to remove other plugin or core hook handlers, could it not be up to the plugins or core to create patterns around doing so, without needlessly bloating the argument signature of the hook creators?

Alternatively, even if we were to keep the direction we'd explored here, we might have considered some helpers to help create scoping to avoid repetition, like hook creators.

const pluginHookCreator = createHookCreator( 'vendor/myplugin' );
pluginHookCreator.addAction( 'actionName', callback );

Or perhaps something integrated into build tooling that eliminates the need to explicitly specify the vendor, namespace. I'd like to explore similar for JavaScript i18n string extraction.

In its current state, I'm not a fan of the approach here. Maybe we can discuss again in tomorrow's JS meeting.

@pento

This comment has been minimized.

Copy link
Member

commented Sep 5, 2017

This needs more than just time to soak in trunk before 4.9, it's a huge new API.

Please consider this a blocker: before it's merged, it needs to be integrated into a substantial project to help iron out practical problems that are likely to come up.

@rmccue

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2017

In place of naming to remove hooks, I'd rather we had taken the direction of some precedent:

My concern with these approaches is that none of them solve the plugin-unhooking-other-plugin problem. Anything that isn't a static identifier is going to fail for that use case. This is a pretty significant use case in PHP-land; I don't see that there are any fundamental differences that would change that in JS-land.

The PHP-land solution is that you always hook something that's globally-exposed, whether that's a function name (always global) or an object method (where you need to expose the object yourself). The problem in JS-land is that functions are not usually globally-exposed, or are completely anonymous, which means we have basically no ID to use. (Anonymous functions for PHP hooks are already discouraged as a best practice for exactly this reason.)

@aduth

This comment has been minimized.

Copy link
Member

commented Sep 5, 2017

My concern with these approaches is that none of them solve the plugin-unhooking-other-plugin problem.

You'll have to forgive my ignorance, but this is not a pattern I've seen very often. Can you point to a few examples so I have a better idea of their use-case?

I'm not anti-ad-hoc-removal (well, maybe a bit 😄 ), but it's a decision of best compromise: Would I sacrifice a built-in option to remove other plugin's hooks if it meant I'd only need to provide a hook name and callback to register my own, rather than hook name, vendor name, namespace, callback name, and callback? For me, yes I would.

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2017

@pento what would you think about merging as an experimental API, similar to what we did with Heartbeat?

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2017

You'll have to forgive my ignorance, but this is not a pattern I've seen very often. Can you point to a few examples so I have a better idea of their use-case?

An example would be you have installed some plugin that filters post content in some way and you want to use the plugin, but remove its content filtering, perhaps in a certain context only.

@pento

This comment has been minimized.

Copy link
Member

commented Sep 6, 2017

what would you think about merging as an experimental API, similar to what we did with Heartbeat?

I'm exceedingly skeptical of that option. Heartbeat being merged as an experimental API didn't really achieve any of its goals: it didn't gain much adoption, the adoption it did gain meant we struggled to break backwards compatibility, and it didn't evolve much in subsequent releases.

I think the most successful new API in recent years was the REST API core - it was solid, having been used as the base for the REST API endpoints, there have been lots of examples of plugins building new endpoints based on it, and we haven't had huge issues with needing to break backwards compatibility.

@aaronjorbin

This comment has been minimized.

Copy link
Member

commented Sep 6, 2017

My concern with these approaches is that none of them solve the plugin-unhooking-other-plugin problem.

You'll have to forgive my ignorance, but this is not a pattern I've seen very often. Can you point to a few examples so I have a better idea of their use-case?

A quick grep of the plugin repo shows many examples. Here is one where a plugin looks to see if woocomerce breadcrumbs are enabled and if so, replaces the action with one of it's own: https://plugins.trac.wordpress.org/browser/catalyst-connect/trunk/inc/frontend.php#L104

Here is an example of modifying the genesis theme: https://plugins.trac.wordpress.org/browser/thememix-pro-genesis/trunk/page-templates/page-login.php#L14

This technique was also talked about as a part of how NPR built the Argo network: https://wordpress.tv/2011/09/25/marc-lavallee-wes-lindamood-plugins-are-blueprints/

@aaronjorbin

This comment has been minimized.

Copy link
Member

commented Sep 6, 2017

It appears the goal is to turn hook names into PHP namespaces. If so, I think we need about 7 more slashes to really replicate the experience.

As with all large API changes (Gutenberg, REST-API, etc), this should have a long period of relative stability with solid documentation in order to develop maturity before it is added to core.

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 7, 2017

As with all large API changes (Gutenberg, REST-API, etc), this should have a long period of relative stability with solid documentation in order to develop maturity before it is added to core.

Worth noting that the code in this package was originally provided on https://core.trac.wordpress.org/ticket/21170 over 5 years ago. Although we have added some tests and small improvements in the last few months, the core of the library remains unchanged.

In regards to documentation, here is what we have so far: https://github.com/adamsilverstein/packages/blob/45747e5ad147a6b35f2b7bb508eb139a475e2094/packages/hooks/README.md - this could definitely be improved with additional details and examples.

This needs more than just time to soak in trunk before 4.9, it's a huge new API.
Please consider this a blocker: before it's merged, it needs to be integrated into a substantial project to help iron out practical problems that are likely to come up.

Really? This seems pretty solid and well tested to me, and worth having in core as we explore ways to make our JavaScript more extensible, for example the media manager. I am ready to commit this code on https://core.trac.wordpress.org/ticket/21170 - it is mainly 5+ year old code that has had review from project leads and some of our top core JS engineers. This is a solid, reliable hooks library I would use in production today. Sure, we may still find edge cases bugs: let's start using it to find out.

Lets release this as an NPM package, then projects like Gutenberg or WordPress core itself can test it out by importing it from npm.

the readme is redone, can you please re-review?

@azaozz

This comment has been minimized.

Copy link

commented Sep 8, 2017

Argh, the context from https://core.trac.wordpress.org/ticket/21170 is so much missing here, or rather this discussion should have been at the core trac ticket, where it belongs :(

Not sure if this is a "big new API", it is a pub/sub that replaces jQuery( document ).trigger(). As @adamsilverstein mentions above, it has been waiting in 21170 for over 5 years already. Don't think it can be compared to the REST API both as size and as complexity.

Heartbeat was marked "experimental" not because it needed to "wait" for plugins to start using it. There were two main reasons:

  • Some people were concerned that a request every 30 seconds was too much for some cheap hosting accounts to handle. (Btw I'd really want to see what would happen when the REST API gets a decent amount of usage on all installs).
  • The browser support for detecting when the user has switched tabs, focused an iframe, or moved away/became inactive was sketchy, so there were several workarounds that worked in different browsers to different degree.

Think it's about time to bite the bullet and finally commit this. The alternative is to continue using jQuery( document ).trigger() in core, no better in any way. Also, five years is a very very long time no matter how you look at it :)

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 12, 2017

wp.hooks has landed in core, let's get this package released to npm as 1.0.0.

https://core.trac.wordpress.org/changeset/41375

@adamsilverstein

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 18, 2017

Let's get this published on npm as 0.1.0-beta.0, that way we can try using via import in Gutenberg and WordPress core.

@adamsilverstein adamsilverstein merged commit 18fed13 into WordPress:master Sep 22, 2017

1 of 3 checks passed

codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.