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

doAction when a deprecated feature is encountered #8110

Merged
merged 15 commits into from Aug 30, 2018

Conversation

@aaronjorbin
Member

aaronjorbin commented Jul 21, 2018

Description

Whenever a deprecated feature is encountered, an action should be fired. This is necessary for log deprecated notices to be able to easily log these deprecations or for other plugins to handle and do something when somethign deprecarted is encountered.

How has this been tested?

Added this js to the page:
wp.hooks.addAction( 'wp.deprecated', 'yo.dawg', function( feature, options, message ){ alert( 'DEPRECATED!' ); console.log( feature, options, message ); });

There is also a unit test. ¯\(ツ)

Types of changes

This adds a new dependency to the deprecated package. Not sure if this needs to be tested specially with packages or we need to do something special.

Checklist:

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

@aaronjorbin aaronjorbin changed the title from WIP: doAction when a deprecated feature is encountered to doAction when a deprecated feature is encountered Jul 22, 2018

@aaronjorbin

This comment has been minimized.

Show comment
Hide comment
@aaronjorbin

aaronjorbin Jul 22, 2018

Member

Simple code I used to test this:

wp.hooks.addAction( 'wp.deprecated', 'yo.dawg', function( feature, options, message ){
alert( 'DEPRECATED!' );
console.log( feature, options, message );
});

Member

aaronjorbin commented Jul 22, 2018

Simple code I used to test this:

wp.hooks.addAction( 'wp.deprecated', 'yo.dawg', function( feature, options, message ){
alert( 'DEPRECATED!' );
console.log( feature, options, message );
});

Show outdated Hide outdated packages/deprecated/src/index.js
@@ -1,3 +1,4 @@
import { doAction } from '@wordpress/hooks';

This comment has been minimized.

@gziolo

gziolo Jul 23, 2018

Member

It should have:

/**
 * WordPress dependencies
 */

comment above.

@gziolo

gziolo Jul 23, 2018

Member

It should have:

/**
 * WordPress dependencies
 */

comment above.

This comment has been minimized.

@aaronjorbin
@aaronjorbin
* Fires whenever a deprecated feature is encountered
*
* @param {string} feature Name of the deprecated feature.
* @param {?Object} options Personalisation options

This comment has been minimized.

@gziolo

gziolo Jul 23, 2018

Member

I'm wondering if there is a way to turn options into shape to avoid duplication. I saw something like this in the past:
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/autocomplete/index.js#L93-L105

@gziolo

gziolo Jul 23, 2018

Member

I'm wondering if there is a way to turn options into shape to avoid duplication. I saw something like this in the past:
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/autocomplete/index.js#L93-L105

This comment has been minimized.

@aaronjorbin

aaronjorbin Jul 25, 2018

Member

My thinking here was that in PHP actions have a documentation standard. https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#4-hooks-actions-and-filters and I was trying to copy that here. I guess we don't have a standard for hooks documentation in JS yet?

@aaronjorbin

aaronjorbin Jul 25, 2018

Member

My thinking here was that in PHP actions have a documentation standard. https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#4-hooks-actions-and-filters and I was trying to copy that here. I guess we don't have a standard for hooks documentation in JS yet?

This comment has been minimized.

@gziolo

gziolo Jul 26, 2018

Member

Yes, we didn't come up with any standards around that. It might make sense to follow PHP implementation though.

@gziolo

gziolo Jul 26, 2018

Member

Yes, we didn't come up with any standards around that. It might make sense to follow PHP implementation though.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jul 23, 2018

Member

It looks like a good addition which would allow to integrate it with other parts of WordPress. Let's run it through weekly JS chat to make sure everybody is on board with the idea.

Member

gziolo commented Jul 23, 2018

It looks like a good addition which would allow to integrate it with other parts of WordPress. Let's run it through weekly JS chat to make sure everybody is on board with the idea.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jul 23, 2018

Member

Let's run it through weekly JS chat to make sure everybody is on board with the idea.

Added to agenda.

Member

aduth commented Jul 23, 2018

Let's run it through weekly JS chat to make sure everybody is on board with the idea.

Added to agenda.

@adamsilverstein

Haven't actually tested code, but overall I love this idea. Great work @aaronjorbin!

@gziolo

Can we also add documentation in README file and a unit test before we merge this one?

* Fires whenever a deprecated feature is encountered
*
* @param {string} feature Name of the deprecated feature.
* @param {?Object} options Personalisation options

This comment has been minimized.

@gziolo

gziolo Jul 26, 2018

Member

Yes, we didn't come up with any standards around that. It might make sense to follow PHP implementation though.

@gziolo

gziolo Jul 26, 2018

Member

Yes, we didn't come up with any standards around that. It might make sense to follow PHP implementation though.

Show outdated Hide outdated packages/deprecated/src/index.js
Show outdated Hide outdated packages/deprecated/package.json
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jul 30, 2018

Member

What's the process by which we should decide when it's appropriate to introduce a new action? Is it simply a "that could be useful" consensus? Is there documentation for this even in how PHP hooks are introduced?

Edit: To be clear, I'm not objecting here. It just seems like we've been very ad hoc and inconsistent in how we're using hooks, which will be prone to misunderstanding and confusion.

Member

aduth commented Jul 30, 2018

What's the process by which we should decide when it's appropriate to introduce a new action? Is it simply a "that could be useful" consensus? Is there documentation for this even in how PHP hooks are introduced?

Edit: To be clear, I'm not objecting here. It just seems like we've been very ad hoc and inconsistent in how we're using hooks, which will be prone to misunderstanding and confusion.

@aaronjorbin

This comment has been minimized.

Show comment
Hide comment
@aaronjorbin

aaronjorbin Aug 13, 2018

Member

What's the process by which we should decide when it's appropriate to introduce a new action? Is it simply a "that could be useful" consensus? Is there documentation for this even in how PHP hooks are introduced?

There isn't a formal one for PHP Hooks right now. It's generally up to the committers if they think that the use case is one that can only be accomplished if a new hook is added and if that hook is unlikely in their mind to cause a loss of flexibility in the future.

Member

aaronjorbin commented Aug 13, 2018

What's the process by which we should decide when it's appropriate to introduce a new action? Is it simply a "that could be useful" consensus? Is there documentation for this even in how PHP hooks are introduced?

There isn't a formal one for PHP Hooks right now. It's generally up to the committers if they think that the use case is one that can only be accomplished if a new hook is added and if that hook is unlikely in their mind to cause a loss of flexibility in the future.

@aaronjorbin

This comment has been minimized.

Show comment
Hide comment
@aaronjorbin

aaronjorbin Aug 13, 2018

Member

@gziolo Documentation and a unit test have been added.

Member

aaronjorbin commented Aug 13, 2018

@gziolo Documentation and a unit test have been added.

@gziolo gziolo added this to the 3.7 milestone Aug 22, 2018

@gziolo

My only remaining concern is the name of the action. In other places when we set up hooks we prefix them with the name of the package. In this case, the function is the default export for the package so I think it should be simply:

doAction( 'deprecated', feature, options, message );

Let's agree on that before we merge. It would be nice to include an actual example in README.md file, too. Otherwise, it looks great.

Show outdated Hide outdated packages/deprecated/README.md
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 30, 2018

Contributor

How do we proceed here, can we update the action name to deprecated to match the other actions we have (until we reach a consensus on the conventions). We're trying to release 3.7 today, so might be good to update to ship in the release.

Contributor

youknowriad commented Aug 30, 2018

How do we proceed here, can we update the action name to deprecated to match the other actions we have (until we reach a consensus on the conventions). We're trying to release 3.7 today, so might be good to update to ship in the release.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 30, 2018

Member

I will take care of it 👍

Member

gziolo commented Aug 30, 2018

I will take care of it 👍

@gziolo

gziolo approved these changes Aug 30, 2018

I addressed my own comments and adjusted the hook name according to the chat we had earlier this week during Core JS weekly meeting.

@gziolo gziolo merged commit a3eb955 into master Aug 30, 2018

2 checks passed

codecov/project 50.23% (+<.01%) compared to 774ccda
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the feature/deprecation-action branch Aug 30, 2018

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 30, 2018

Member

Thanks @aaronjorbin for leading this PR 🙇

Member

gziolo commented Aug 30, 2018

Thanks @aaronjorbin for leading this PR 🙇

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