Hooks: Use own instance's doAction for built-in hooks
#26498
Conversation
|
@adamsilverstein - would you mind checking this bug report? |
packages/hooks/src/createHooks.js
Outdated
| const hooks = Object.create( null ); | ||
|
|
||
| Object.assign( hooks, { | ||
| addAction: createAddHook( actions ).bind( hooks ), |
There was a problem hiding this comment.
The actions and filters objects should be properties of the parent hook object, rather than being related together just by closure variable references.
This also avoids having to call bind() on the return value of every create* function. The bind call will still be there, but hidden inside the create* implementation.
addAction: createAddHook( hooks, 'actions' );etc.
There was a problem hiding this comment.
Do you think it'd be better update it here, or separately in a follow-up PR?
There was a problem hiding this comment.
@yuliyan I think making this update will also help fix the type checking errors reported by @adamsilverstein ('this' implicitly has type 'any'). So you probably want to do it right away 🙂
|
Hi @jsnajdr and thanks for the bug report. I have some questions: Is this a bug or more of an enhancement? eg. before your change are you saying the My only other question is about performance, looking at the travis run, it looks fine/unchanged or better. |
|
@jsnajdr I see some errors when trying to build the PR: Can you fix those issues in your branch? |
|
Hi @adamsilverstein,
I consider this more of a bug, and there are a few reasons to see it like so:
Yes, added tests would fail, as these are covering whether private hooks instances would dispatch
There should be little to no affect on the performance, since the shared global instance and its methods are no different than a private instance.
I'm unable to reproduce these errors by running |
I agree with @yuliyan that this fixes a bug. The |
@yuliyan |
@jsnajdr, doesn't Anyway, I tried with |
|
@yuliyan OK then the problem is something else: it was only in #26430, merged on Nov 3, when the |
yuliyan
force-pushed
the
fix/hooks-use-own-do-action
branch
from
fe8d52c
to
30d105b
Compare
|
@adamsilverstein, I've reworked the hooks referencing in create* functions as suggested above by @jsnajdr, which also fixed the type errors. @jsnajdr, could you also take a look and let me know if you spot anything that can be improved? |
|
The type of the object returned by Before, the After the patch, the object is just a generic bag of functions ( |
|
I tried to type the The "hook registry" instance, returned by That makes the The only way I found to make TypeScript happy is to declare a public interface and then define a class that implements it: type HookType = "filters" | "actions";
type Hook = boolean;
export interface Hooks {
filters: Record<string, Hook>;
actions: Record<string, Hook>;
addFilter(name: string): void;
addAction(name: string): void;
onAdd(type: HookType, name: string): void;
}
class HooksImpl implements Hooks {
// have to repeat `Hooks` field declarations, otherwise the `HooksImpl` type
// is deemed "incomplete".
filters: Hooks["filters"];
actions: Hooks["actions"];
addFilter: Hooks["addFilter"];
addAction: Hooks["addAction"];
onAdd: Hooks["onAdd"];
constructor() {
this.actions = Object.create(null);
this.addFilter = createAddHook(this, "filters");
this.addAction = createAddHook(this, "actions");
this.onAdd = (type: HookType, name: string) => {
console.log("Added hook:", type, name);
};
}
}
export function createHooks(): Hooks {
return new HooksImpl();
}
function createAddHook(hooks: Hooks, type: HookType) {
return (name: string) => {
hooks[type][name] = true;
hooks.onAdd(type, name);
};
}One notable attempt that doesn't work: function createHooks(): Hooks {
const hooks = {};
hooks.addFilter = createAddHook( hooks, "filters" );
/* ... */
return hooks;
}It fails with this error: It's a Catch 22: I can't pass If the solution with |
|
A simpler implementation that doesn't use interface and implementing class, but just a class: class HooksClass {
filters: Record<string, Hook>;
actions: Record<string, Hook>;
addFilter: (name: string) => void;
addAction: (name: string) => void;
onAdd: (type: HookType, name: string) => void;
constructor() {
this.actions = Object.create(null);
this.addFilter = createAddHook(this, "filters");
this.addAction = createAddHook(this, "actions");
this.onAdd = (type: HookType, name: string) => {
console.log("Added hook:", type, name);
};
}
}
export type Hooks = InstanceType<typeof HooksClass>;
export function createHooks(): Hooks {
return new HooksClass();
}The |
|
@jsnajdr, I'm trying to wrap my head around your suggestion and I'm unsure if HookClass properties are supposed to get their type definitions from the return type of the factory function they are initialized with in the constructor, but when I tested it, it seems like they remain with their initial type, e.g.: I'm currently experimenting a different approach to manually define the hooks instance using the callback definitions, i.e.: |
I think there is a big difference in how TypeScript treats classes and other objects. If On the other hand, the function createHooks() {
return {
addFilter: createAddHook( filters ),
addAction: createAddHook( actions ),
}
}and then the return type of But that no longer works once we want to pass the function createHooks() {
const hooks = {
addFilter: createAddHook( hooks, filters ),
addAction: createAddHook( hooks, actions ),
};
return hooks;
}And this doesn't work because when the object is constructed incrementally, TypeScript complains that the value passed to function createHooks() {
const hooks = {};
hooks.addFilter = createAddHook( hooks, filters );
hooks.addAction = createAddHook( hooks, actions );
return hooks;
}Using a class is the only way out of this as far as I know. When the "incremental construction" happens in a class constructor, TypeScript is happy with that: constructor() {
this.addFilter = createAddHook( this, filters );
this.addAction = createAddHook( this, actions );
} |
|
I like @jsnajdr's suggestion, a class nicely represents the Hooks object we want - it encapsulates related data and methods. We just expose a I went ahead and implemented the suggestion with JSDoc types, here's the diff: class implementationdiff --git a/packages/hooks/src/createHooks.js b/packages/hooks/src/createHooks.js
index 27fe8b3824..361383a3a9 100644
--- a/packages/hooks/src/createHooks.js
+++ b/packages/hooks/src/createHooks.js
@@ -9,42 +9,51 @@ import createCurrentHook from './createCurrentHook';
import createDoingHook from './createDoingHook';
import createDidHook from './createDidHook';
+/**
+ * Internal class for constructing hooks. Use `createHooks()` function
+ *
+ * Note, it is necessary to expose this class to make its type public.
+ *
+ * @private
+ */
+export class _Hooks {
+ constructor() {
+ /** @type {import('.').Store} actions */
+ this.actions = Object.create( null );
+ this.actions.__current = [];
+
+ /** @type {import('.').Store} filters */
+ this.filters = Object.create( null );
+ this.filters.__current = [];
+
+ this.addAction = createAddHook( this, 'actions' );
+ this.addFilter = createAddHook( this, 'filters' );
+ this.removeAction = createRemoveHook( this, 'actions' );
+ this.removeFilter = createRemoveHook( this, 'filters' );
+ this.hasAction = createHasHook( this, 'actions' );
+ this.hasFilter = createHasHook( this, 'filters' );
+ this.removeAllActions = createRemoveHook( this, 'actions', true );
+ this.removeAllFilters = createRemoveHook( this, 'filters', true );
+ this.doAction = createRunHook( this, 'actions' );
+ this.applyFilters = createRunHook( this, 'filters', true );
+ this.currentAction = createCurrentHook( this, 'actions' );
+ this.currentFilter = createCurrentHook( this, 'filters' );
+ this.doingAction = createDoingHook( this, 'actions' );
+ this.doingFilter = createDoingHook( this, 'filters' );
+ this.didAction = createDidHook( this, 'actions' );
+ this.didFilter = createDidHook( this, 'filters' );
+ }
+}
+
+/** @typedef {_Hooks} Hooks */
+
/**
* Returns an instance of the hooks object.
+ *
+ * @return {Hooks} A Hooks instance.
*/
function createHooks() {
- /** @type {import('.').Store} */
- const actions = Object.create( null );
- /** @type {import('.').Store} */
- const filters = Object.create( null );
- actions.__current = [];
- filters.__current = [];
-
- /** @type {import('.').Hooks} */
- const hooks = Object.create( null );
-
- Object.assign( hooks, {
- addAction: createAddHook( hooks, 'actions' ),
- addFilter: createAddHook( hooks, 'filters' ),
- removeAction: createRemoveHook( hooks, 'actions' ),
- removeFilter: createRemoveHook( hooks, 'filters' ),
- hasAction: createHasHook( hooks, 'actions' ),
- hasFilter: createHasHook( hooks, 'filters' ),
- removeAllActions: createRemoveHook( hooks, 'actions', true ),
- removeAllFilters: createRemoveHook( hooks, 'filters', true ),
- doAction: createRunHook( hooks, 'actions' ),
- applyFilters: createRunHook( hooks, 'filters', true ),
- currentAction: createCurrentHook( hooks, 'actions' ),
- currentFilter: createCurrentHook( hooks, 'filters' ),
- doingAction: createDoingHook( hooks, 'actions' ),
- doingFilter: createDoingHook( hooks, 'filters' ),
- didAction: createDidHook( hooks, 'actions' ),
- didFilter: createDidHook( hooks, 'filters' ),
- actions,
- filters,
- } );
-
- return hooks;
+ return new _Hooks();
}
export default createHooks;
diff --git a/packages/hooks/src/index.js b/packages/hooks/src/index.js
index ada041d3d1..bf6a6a9834 100644
--- a/packages/hooks/src/index.js
+++ b/packages/hooks/src/index.js
@@ -26,14 +26,8 @@ import createHooks from './createHooks';
/**
* @typedef {Record<string, Hook> & {__current: Current[]}} Store
- */
-
-/**
* @typedef {'actions' | 'filters'} StoreKey
- */
-
-/**
- * @typedef {Record<string, Function> & Record<StoreKey, Store>} Hooks
+ * @typedef {import('./createHooks').Hooks} Hooks
*/
const {
|
|
The patched
But there is one change in the types that could be considered as breaking: before this patch, the After this patch, the return type of |
As far as I can tell, the typed hook package hasn't been released yet so this is an extension of the original enhancement of adding types. The latest release does not appear to include |
Perfect! That makes the PR ready to ship in my view 👍 |
|
Congratulations on your first merged pull request, @yuliyan! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
Description
Hooks are currently using
doActionfrom the shared global instance to trigger the built-in hookshookAddedandhookRemoved. This PR binds the methods for adding and removing hooks to their own hooks instance so that they usedoActionfrom the given instance for the said built-in hooks.How has this been tested?
Tests pass successfully.
Types of changes
Bug fix: Use own instance's
doActionmethod to trigger built-inhookAddedandhookRemovedhooks.Checklist: