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

Framework: Adding a deprecation helper #5069

Merged
merged 2 commits into from Feb 19, 2018

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented Feb 15, 2018

This PR adds a deprecated function to produce a consistent warning message when deprecating features in WordPress and WordPress plugin.

You can provide:

  • A feature name (the only mandatory parameter),
  • A version in which the feature will be deprecated,
  • An alternative feature to use instead,
  • A plugin name (default to WordPress Core)
  • A link for more details

@youknowriad youknowriad self-assigned this Feb 15, 2018

@youknowriad youknowriad requested a review from aduth Feb 15, 2018

'Deprecated: `select` now accepts only a single argument: the reducer key. ' +
'The return value is an object of selector functions.'
);
deprecated( 'Calling select with multiple arguments', '2.4', undefined, 'Gutenberg' );

This comment has been minimized.

@gziolo

gziolo Feb 15, 2018

Member

Have you consider using an object with named params instead? It would help to avoid passing undefined and make it easier to reason which param does what when reading code.

This comment has been minimized.

@youknowriad

youknowriad Feb 15, 2018

Contributor

Yes, I was hesitant because the calling side will be more verbose. I don't have a strong opinion though. Happy to update with what y'all think is best.

This comment has been minimized.

@gziolo

gziolo Feb 15, 2018

Member

Yes, it'd produce more work for developers :(

This comment has been minimized.

@westonruter

westonruter Feb 15, 2018

Member

I agree with passing an object param rather than positional params. WordPress is rife with functions that contain positional parameters which are hard to remember the order of. In some cases a positional parameter has become deprecated requiring that it be change to $deprecated in the function signature, and requires always passing null in each call. Using an object is more flexible and will allow adding additional parameters in the future as required.

*/
import { deprecated } from '../deprecation';
describe( 'deprecated', () => {

This comment has been minimized.

@gziolo

gziolo Feb 15, 2018

Member

Love those tests, they are so fun to read and perfectly explain all the logic hidden in the tested method ❤️

@gziolo

gziolo approved these changes Feb 15, 2018

I love this PR, it provides a unified way to handle deprecation logic. It gives a very user-friendly message with all required details and is nicely covered with tests. Great work @youknowriad. I had one concern regarding params, but I'm aware that there are pros and cons of both approaches. Just wanted to share my feeling from the first pass on this code.

@aduth

This comment has been minimized.

Member

aduth commented Feb 15, 2018

WordPress is rife with functions that contain positional parameters which are hard to remember the order of.

This is my concern with the approach here, and the obvious alternative of changing to an object shape makes me start to wonder if we ought to just simplify this to:

deprecated( version, message )

...Even if it means losing some of the automatic consistency.

Maybe message could look like:

Deprecated: [message]. This will be removed in [version].

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 16, 2018

I do think having a helper with just the version and the message doesn't bring too much value in term of consistency but I agree with the arguments issue we have right now.

What do you all think with an update API like this:

deprecated( feature, { version, alternative, plugin, link } )

The feature is the only mandatory argument, it makes sense to leave it as a separate argument and the others are moved to an object.

@gziolo

This comment has been minimized.

Member

gziolo commented Feb 19, 2018

Nice, even better than before 👍

@youknowriad youknowriad merged commit da8184a into master Feb 19, 2018

2 checks passed

codecov/project 34.43% (+0.27%) compared to 013bf26
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/deprecation-helper branch Feb 19, 2018

@aduth

This comment has been minimized.

Member

aduth commented Feb 19, 2018

I don't particularly love the fact that the developer needs to understand how the strings are concatenated into the final output sentence. If we were to have recommendations as part of the API, I think it might be less confusing to have developer provide these as full sentences, then output like:

Deprecated: [feature]

Recommendation: [recommendation]
To be removed in: [plugin] [version]

The biggest win I'd see in having a helper like this is enforcing a predictable timeline for removal. Currently nothing will happen after e.g. v2.4 or v2.5 is released.

I might imagine one of:

  • We include disabling as part of the helper (if ( deprecated( ... ) ) { return; })
  • We include as part of the release process a check to ensure that all deprecated usage for the current release is removed (and presumably deprecated functionality with it)
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 19, 2018

@aduth Makes sense to me.

We include as part of the release process a check to ensure that all deprecated usage for the current release is removed (and presumably deprecated functionality with it)

I thought it could be manual at first but would be great to provide an automatic way to do so.

Deprecated: [feature]

Recommendation: [recommendation]
To be removed in: [plugin] [version]

I like this format and agree with your statement. Feel free to update the helper. I could also circle back later.

Thanks

@gziolo

This comment has been minimized.

Member

gziolo commented Feb 19, 2018

We include disabling as part of the helper (if ( deprecated( ... ) ) { return; })

That's a really interesting idea 👍

@aduth

This comment has been minimized.

Member

aduth commented Feb 19, 2018

I think the automation could be achieved as part of a version-bump handler which performs all of the current tasks necessary for a version bump, plus updating a no-restricted-syntax rule matching deprecations which must be removed.

With the object format, it's a little more difficult, but if we had it as an argument:

https://astexplorer.net/#/gist/f557598203a267fca7857544ddef786b/d53579d35af925fafdae913dc772ea6104843f0a

The AST selector could look something like:

CallExpression[callee.name="deprecated"][arguments[1].value="2.4"]

Or we could just create a custom rule which extracts the plugin version from e.g. package.json.

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