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

Extensibility: Apply filters without function wrappers #3933

Merged
merged 2 commits into from Dec 14, 2017

Conversation

Projects
None yet
2 participants
@gziolo
Member

gziolo commented Dec 12, 2017

Description

Raised by @aduth when working on converting all filters for the components to work as HOCs (#3827 (comment)):

Minor: Do we need to export these as functions anymore, or can they just call to addFilter when imported?

This PR explores if we can avoid exporting functions that enable filters for blocks. Instead we apply them directly in the same file when corresponding callbacks are defined.

How Has This Been Tested?

Unit tested.

Types of changes

Refactoring.

Checklist:

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

@gziolo gziolo self-assigned this Dec 12, 2017

@gziolo gziolo requested a review from aduth Dec 12, 2017

@gziolo gziolo changed the title from Update/hooks without wrappers to Extensibility: Apply filters without function wrappers Dec 12, 2017

@@ -6,41 +6,31 @@ import { noop } from 'lodash';
/**
* WordPress dependencies
*/
import { applyFilters, removeAllFilters } from '@wordpress/hooks';

This comment has been minimized.

@aduth

aduth Dec 12, 2017

Member

So the downside to this approach is that we can't clean up lingering handlers after tests?

@aduth

aduth Dec 12, 2017

Member

So the downside to this approach is that we can't clean up lingering handlers after tests?

This comment has been minimized.

@gziolo

gziolo Dec 12, 2017

Member

Test files are isolated, so they should be cleaned up anyways. I hope 😄

@gziolo

gziolo Dec 12, 2017

Member

Test files are isolated, so they should be cleaned up anyways. I hope 😄

This comment has been minimized.

@aduth

aduth Dec 12, 2017

Member

Test files are isolated, so they should be cleaned up anyways. I hope 😄

Hmm, the require cache is cleared between each? Would be (good) news to me if true.

@aduth

aduth Dec 12, 2017

Member

Test files are isolated, so they should be cleaned up anyways. I hope 😄

Hmm, the require cache is cleared between each? Would be (good) news to me if true.

This comment has been minimized.

@gziolo

gziolo Dec 12, 2017

Member

I think it isn’t the case only for mocks imported in Jest setup file.

You can enable reseting modules per test using resetModules config option or call it programmatically with the method called the same.

@gziolo

gziolo Dec 12, 2017

Member

I think it isn’t the case only for mocks imported in Jest setup file.

You can enable reseting modules per test using resetModules config option or call it programmatically with the method called the same.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 14, 2017

Member

@aduth should I proceed with this PR? Let me know if you like it more? I'm convinced :)

Member

gziolo commented Dec 14, 2017

@aduth should I proceed with this PR? Let me know if you like it more? I'm convinced :)

@aduth

aduth approved these changes Dec 14, 2017

@gziolo gziolo merged commit 79c3f34 into master Dec 14, 2017

3 checks passed

codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +61.75% compared to 050ea62
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gziolo gziolo deleted the update/hooks-without-wrappers branch Dec 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment