Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wp.hooks - enable the 'all' hook #12038

Merged
merged 9 commits into from
May 16, 2019
Merged

wp.hooks - enable the 'all' hook #12038

merged 9 commits into from
May 16, 2019

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Nov 18, 2018

Description

Fixes #8602

How has this been tested?

First pass: needs testing.

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo requested a review from aduth February 5, 2019 14:31
@gziolo gziolo added [Package] Hooks /packages/hooks [Type] Task Issues or PRs that have been broken down into an individual action to take Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Enhancement A suggestion for improvement. and removed [Type] Task Issues or PRs that have been broken down into an individual action to take labels Feb 5, 2019
@gziolo
Copy link
Member

gziolo commented Feb 5, 2019

What's the status of this one. Is it still something that it expected to be included in the library?

@adamsilverstein
Copy link
Member Author

What's the status of this one. Is it still something that it expected to be included in the library?

Yes, this seems genuinely useful (maybe essential) for debugging hooks.

I'll work on adding some tests and exploring the monkey patching approach forward. To introduce this feature, we need to insure it has minimal or no impact on performance.

One other option I might explore instead of monkey patching is a debug version of the build that would include the 'all' hook support and would be loaded when SCRIPT_DEBUG is true. that would likely be a cleaner approach and would result in zero impact on production performance. Since this feature is intended for debugging only, this might make the most sense anyway.

What do you think?

@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

One other option I might explore instead of monkey patching is a debug version of the build that would include the 'all' hook support and would be loaded when SCRIPT_DEBUG is true. that would likely be a cleaner approach and would result in zero impact on production performance. Since this feature is intended for debugging only, this might make the most sense anyway.

Sounds great to be disabled by default but enabled in debug mode. I'm wondering how it could be distributed to npm to take advantage of it and ensure that dead code is never bundled for production.

@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Feb 6, 2019
@adamsilverstein
Copy link
Member Author

Sounds great to be disabled by default but enabled in debug mode. I'm wondering how it could be distributed to npm to take advantage of it and ensure that dead code is never bundled for production.

How about using the dev/prod build flag so we build the minified production bundle without the 'all' support, while building the unminified version with the 'all' hook support?

I'll try to work on this.

@adamsilverstein
Copy link
Member Author

adamsilverstein commented Feb 6, 2019

This small change may be all that is required. I am going to write some tests against this functionality to see if it works as expected. The performance penalty should be very small in practice, when no all hooks have been registered a single check is added (admittedly to every apply hooks call). The code is wrapped in comment blocks formatted with an eye towards webpack-strip-block - thinking we can strip this off when we create the minified production version of hooks for core.

@adamsilverstein adamsilverstein added Needs Tests [Type] WP Core Ticket Requires an upstream change from WordPress. Core Trac ticket should be linked. labels Feb 6, 2019
/* develblock:start */
// Handle any 'all' hooks registered.
if ( hooks.all ) {
handlers.push( hooks.all.handlers );
Copy link
Member

@aduth aduth Feb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're pushing an array into the array, rather than the members of that array.

i.e.

handlers = [ 1 ];
hooks.all.handlers = [ 2 ];

handlers.push( hooks.all.handlers );

console.log( handlers );
// [ 1, [ 2 ] ]

You'd want one of either:

handlers = handlers.concat( hooks.all.handlers );
handlers.push( ...hooks.all.handlers );

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eagle eyes! i wouldn't have caught that error until i wrote the tests. will fix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fixed.

@@ -30,6 +30,14 @@ function createRunHook( hooks, returnFirstArg ) {

const handlers = hooks[ hookName ].handlers;

// The following code can be stripped from production builds.
/* develblock:start */
Copy link
Member

@aduth aduth Feb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be processed by a Webpack plugin we're not using?

https://www.npmjs.com/package/webpack-strip-block

We may consider instead a combination of Webpack's (built-in) DefinePlugin and dead-code elimination from its (built-in) minification:

https://webpack.js.org/plugins/define-plugin/

Prior art:

  • // In test environments, pre-process the sprintf message to improve
    // readability of error messages. We'd prefer to avoid pulling in this
    // dependency in runtime environments, and it can be dropped by a combo
    // of Webpack env substitution + UglifyJS dead code elimination.
    if ( process.env.NODE_ENV === 'test' ) {
    return ( ...args ) => logger( require( 'sprintf-js' ).sprintf( ...args ) );
    }
  • Add feature flags for Phase 2 #13324

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very interesting.

i was actually imagining that the all hook handling would remain in the package by default and that we would strip it in the WordPress core build for the minified file.

In core we already build two versions of each script: one for SCRIPT_DEBUG and one for production. We can use these tags and the webpack-strip-block plugin to remove them in core's production build. This would completely eliminate the code for produciton and not introduce the overhead of checking the constant.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The treatment of the environment variable is a bit of a facade, really. It actually achieves the same effect as what you're describing. The way the DefinePlugin works is to literally swap the identifier with some substitute. Thus, if you introduced a condition like this, the body of the condition would be kept for development, and removed for production.

Before (source):

/* a */
if ( process.env.NODE_ENV !== 'production' ) {
	/* b */
}
/* c */

After (built with Webpack, unminified):

/* a */
if ( 'production' !== 'production' ) {
	/* b */
}
/* c */

After (built with Webpack, minified, comments kept for demonstration only):

/* a *//* c */

Copy link
Member Author

@adamsilverstein adamsilverstein Feb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, cool - so we don't need the or the tagging, just a conditional, got it. thanks for clarifying.

So in terms of what we ship in the package, we only ship source right? Do we ship debug scripts with the GB plugin or in core?

my ultimate goal is to have this depend of a wp-config constant, ideally SCRIPT_DEBUG.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in terms of what we ship in the package, we only ship source right? Do we ship debug scripts with the GB plugin or in core?

My expectation would have been that process.env.NODE_ENV would be effectively "development" for the non-minified scripts we ship with core, and thus if SCRIPT_DEBUG would cause the unminified form to load, it would take effect as you expect. However, considering an example where we console.error a message, I see no reference to said message in the full unminified source for wp.components. I'm not too sure if this is intended or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that process.env.NODE_ENV would work the way it's expected. WordPress core would have to generate two versions of the files: development and production. SCRIPT_DEBUG would pick the proper version then. If this isn't the case in core, we should fix it.

@adamsilverstein
Copy link
Member Author

Added some tests demonstrating the 'all' hook in 92920ff

@adamsilverstein
Copy link
Member Author

cc: @johnbillion this might be useful for query monitor

@gziolo
Copy link
Member

gziolo commented Apr 24, 2019

@adamsilverstein, do you plan to use guard based on process.env.NODE_ENV? I think this is the last thing missing to land this PR.

gziolo
gziolo previously requested changes Apr 24, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR needs also rebase to trigger new Travis services.

@adamsilverstein
Copy link
Member Author

@adamsilverstein, do you plan to use guard based on process.env.NODE_ENV? I think this is the last thing missing to land this PR.

Yes, thanks for the reminder about this ticket. I will work on it.

@gziolo gziolo dismissed their stale review May 2, 2019 07:55

I don’t want to block this one

@adamsilverstein
Copy link
Member Author

Great, thanks for the tip - I'll change that.

@adamsilverstein adamsilverstein changed the title WIP: wp.hooks - enable the 'all' hook wp.hooks - enable the 'all' hook May 2, 2019
@adamsilverstein
Copy link
Member Author

@aduth thanks, that fixed it. This is ready for review.

@adamsilverstein adamsilverstein requested a review from aduth May 2, 2019 17:42
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good now. Should we include docs for all hook and add an entry to CHANGELOG file? It might be worth emphasizing that it works in non-production environments only.

@gziolo gziolo removed [Type] WP Core Ticket Requires an upstream change from WordPress. Core Trac ticket should be linked. Needs Tests [Status] In Progress Tracking issues with work in progress labels May 6, 2019
@adamsilverstein
Copy link
Member Author

Should we include docs for all hook and add an entry to CHANGELOG file? It might be worth emphasizing that it works in non-production environments only.

Yes, I will work on adding that, thanks!

@adamsilverstein
Copy link
Member Author

@gziolo

Can you please review to see if those look good?

@@ -1,3 +1,6 @@
## 2.1.0 (2019-05-16)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## 2.1.0 (2019-05-16)
## Master

@@ -1,3 +1,9 @@
## Master
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated CHANGELOG myself. We are still not set on the proper recommendation here, but there is new emerging pattern to list new changes as coming from master. Probably something good to document. /cc @aduth

@gziolo
Copy link
Member

gziolo commented May 16, 2019

@adamsilverstein - I slightly modified CHANGELOG (see my comment #12038 (comment)). Great work, thanks for wrangling this enhancement 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Extensibility The ability to extend blocks or the editing experience [Package] Hooks /packages/hooks [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wp.hooks: support the special all hook
3 participants