TRY: Support an optional namespace parameter for hasAction/hasFilter #106
Conversation
Codecov Report
@@ Coverage Diff @@
## master #106 +/- ##
==========================================
+ Coverage 64.07% 64.72% +0.65%
==========================================
Files 42 42
Lines 604 618 +14
Branches 118 121 +3
==========================================
+ Hits 387 400 +13
- Misses 175 176 +1
Partials 42 42
Continue to review full report at Codecov.
|
packages/hooks/src/createHasHook.js
Outdated
| * | ||
| * @param {string} hookName The name of the hook to check for. | ||
| * @param {string} hookName The name of the hook to check for. | ||
| * @param {string} namespace Optional. The unique namespace identifying the callback in the form `vendor/plugin/function`. |
There was a problem hiding this comment.
Single spaces between @param and {string} and name
* @param {string} hookName The name of the hook to check for.
* @param {string} namespace Optional. The unique naThere was a problem hiding this comment.
thanks, corrected.
packages/hooks/src/createHasHook.js
Outdated
| if ( 'undefined' !== typeof namespace ){ | ||
| return hookName in hooks && | ||
| hooks[ hookName ].handlers && | ||
| hooks[ hookName ].handlers.map( hook => hook['namespace'] ).includes( namespace ); |
There was a problem hiding this comment.
Array#includes is not polyfilled, and is not supported in older browsers:
Further, there's unnecessary work here to iterate the entire array of handlers, when Array#some serves this purpose to terminate iteration once the first match is encountered:
hooks[ hookName ].handlers.some( hook => hook['namespace'] === namespace );https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some
There was a problem hiding this comment.
Ah, right - thanks for catching that. Will fix.
packages/hooks/src/createHasHook.js
Outdated
| // Use the namespace if provided. | ||
| if ( 'undefined' !== typeof namespace ){ | ||
| return hookName in hooks && | ||
| hooks[ hookName ].handlers && |
There was a problem hiding this comment.
Possibly if no handlers have been added? generally prefer the defensive approach of always checking before using because otherwise everything breaks. even if handlers always set up currently, that might change in the future.
There was a problem hiding this comment.
generally prefer the defensive approach of always checking before using because otherwise everything breaks.
Well, there's defensive, but there's also having a solid understanding of the shapes of data we expect to occur.
There was a problem hiding this comment.
True :) I'll take another look - if handlers is always going to be truthy, i will remove the check.
There was a problem hiding this comment.
removed extraneous check
adamsilverstein
force-pushed
the
feature/second-parameter-for-has-hook
branch
from
e1233d1 to
8d8881b
Compare
adamsilverstein
changed the title
|
@aduth any concerns for merging this, seems useful? |
There was a problem hiding this comment.
This looks good to me.
I think improved parity with equivalent core functions is enough justification to want this to land.
Unless it's urgent, I might suggest waiting 'til after WordPress/gutenberg#7556 to avoid complications with including the new history there.
| // Use the namespace if provided. | ||
| if ( 'undefined' !== typeof namespace ){ | ||
| return hookName in hooks && | ||
| hooks[ hookName ].handlers.some( hook => hook.namespace === namespace ); |
There was a problem hiding this comment.
Minor: This style of arrow function will fail ESLint when merged to Gutenberg.
https://github.com/WordPress/gutenberg/blob/ef5cfe077cb54e2b74881aeb7407b9ed15ebce85/eslint/config.js#L32
https://eslint.org/docs/rules/arrow-parens
| return function hasHook( hookName ) { | ||
| return function hasHook( hookName, namespace ) { | ||
| // Use the namespace if provided. | ||
| if ( 'undefined' !== typeof namespace ){ |
There was a problem hiding this comment.
Style: Space before opening curly brace.
| return hookName in hooks && | ||
| hooks[ hookName ].handlers.some( hook => hook.namespace === namespace ); | ||
| } | ||
|
|
||
| return hookName in hooks; |
There was a problem hiding this comment.
Minor: Regardless of whether namespace is provided, we have this same condition apply, so rather than duplicate, we could move it as a primary check of the function:
if ( ! ( hookName in hooks ) ) {
return false;
}
if ( 'undefined' === typeof namespace ) {
return true;
}
return hooks[ hookName ].handlers.some( ( hook ) => {
return hook.namespace === namespace;
} );
|
@aduth thanks for the feedback, I'll work on these soon |
|
Issue moved to WordPress/gutenberg #7819 via ZenHub |
Fixes #104