Skip to content
This repository has been archived by the owner on Jul 9, 2018. It is now read-only.

Hooks: Update hooks public API to make it possible to apply to wp.hooks directly #45

Merged
merged 1 commit into from
Nov 27, 2017

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Nov 24, 2017

We discussed in during the last Core JS meeting. See: https://wordpress.slack.com/archives/C5UNMSU4R/p1511276154000233.

The idea here is to have @wordpress/hooks === wp.hooks, so we could use it directly in Gutenberg. It was raised while working on hooks integration with Gutenberg: WordPress/gutenberg#3493. The existing implementation created an ambiguity where we weren't able to easily expose hooks as wp.hooks and import using @wordpress/hooks at the same time. This PR opens this possibility by making import statement for @wordpress/hooks identical to what is going to be exposed as wp.hooks:

wp.hooks = require( '@wordpress/hooks' );

@codecov
Copy link

codecov bot commented Nov 24, 2017

Codecov Report

Merging #45 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   97.29%   97.31%   +0.01%     
==========================================
  Files          16       17       +1     
  Lines         148      149       +1     
  Branches       41       41              
==========================================
+ Hits          144      145       +1     
  Misses          4        4
Impacted Files Coverage Δ
packages/hooks/src/index.js 100% <100%> (ø) ⬆️
packages/hooks/src/createHooks.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d62ecb7...19cd4cc. Read the comment docs.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

This looks fine to me, and I personally see value in the exposed singleton instance, obviously for our specific needs but generally as well.

The question becomes: When would a developer consuming @wordpress/hooks want to create their own registry of hooks via createHooks ? Would it be bad for many independent modules who each depend on @wordpress/hooks to share the same registry? If we think so, should we put more emphasis in documentation around encouraging createHooks for libraries?

@gziolo
Copy link
Member Author

gziolo commented Nov 27, 2017

The question becomes: When would a developer consuming @wordpress/hooks want to create their own registry of hooks via createHooks ? Would it be bad for many independent modules who each depend on @wordpress/hooks to share the same registry? If

I think that the currently proposed implementation in Gutenberg uses both approaches. We use createHooks to isolate all filters related to blocks and the global instance of hooks for the editor. I'm still not convinced if that is the best way to move forward, but it aligns with the API we expose :)

wp.blocks.registerBlock + wp.blocks.addFilter vs wp.hooks.addFilter.

Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks.

@gziolo gziolo merged commit 12b5c6c into master Nov 27, 2017
@gziolo gziolo deleted the update/public-api-instance branch November 27, 2017 16:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants