Skip to content
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

Deprecated: Introduce new module with depreaction util #6914

Merged
merged 2 commits into from
May 27, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 23, 2018

Description

Extracted from #6828.

Related issues: #3955, #6594.
Similar PRs: #6658, #6756, #6758.

This PR introduces new @wordpress/deprecated package. We might want to find a better name for it. I'm happy to update it if you have better ideas.

How has this been tested?

npm test

Try wp.deprecated( 'My message' ) in the JS console and make sure that deprecation warning is shown properly.

Screenshots

screen shot 2018-05-23 at 14 34 23

Types of changes

Refactoring.

Checklist:

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

@gziolo gziolo requested review from youknowriad and a team May 23, 2018 12:51
@gziolo gziolo self-assigned this May 23, 2018
@gziolo gziolo added [Type] Code Quality Issues or PRs that relate to code quality [Type] Refactoring labels May 23, 2018
@gziolo gziolo added this to the 3.0 milestone May 23, 2018
@@ -0,0 +1,23 @@
{
"name": "@wordpress/devtools",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really bad at finding names so I'll leave this to others.

Thinking that people could confuse devtools with actual dev tools not used in the code. Do you think we should try to find a name that indicates that it's a runtime dependency? I'm not certain there's a better name, just asking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@aduth
Copy link
Member

aduth commented May 23, 2018

Re: name, why not just import deprecated from '@wordpress/deprecated'; ?

@gziolo gziolo changed the title Devtools: Introduce new module with depreacted util Deprecated: Introduce new module with depreaction util May 24, 2018
@gziolo
Copy link
Member Author

gziolo commented May 24, 2018

Re: name, why not just import deprecated from '@wordpress/deprecated';?

Done 👍

@gziolo
Copy link
Member Author

gziolo commented May 24, 2018

Blocked by #6935 as we want to have this function exposed as wp.deprecated, not wp.deprecated.default. See #6748 for full context.

@gziolo gziolo added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label May 24, 2018
@gziolo gziolo removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label May 27, 2018
@gziolo
Copy link
Member Author

gziolo commented May 27, 2018

This should be good now to merge.

@gziolo gziolo merged commit 00320d8 into master May 27, 2018
@gziolo gziolo deleted the update/devtools-package branch May 27, 2018 13:16
{
selector: 'ImportDeclaration[source.value=/^date$/]',
message: 'Use @wordpress/date as import path instead.',
},
{
selector: 'ImportDeclaration[source.value=/^editor$/]',
message: 'Use @wordpress/editor as import path instead.',
selector: 'ImportDeclaration[source.value=/^deprecated/]',
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This pattern should be /^deprecated$/ (otherwise matching e.g. deprecatedfoo)

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed it too late, it is included together with changes planned for data package. Sorry about that :)

@@ -29,10 +29,6 @@ import {
toAsyncIterable,
} from '../';

jest.mock( '@wordpress/utils', () => ( {
Copy link
Member

Choose a reason for hiding this comment

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

Would be neat to have something like our ESLint deprecated removal rules, except for things like this which aren't necessarily "deprecated", but are related to a deprecation or should otherwise be removed by a certain version. Maybe a comment pattern similar to ESLint's inline configuration, e.g. /* REMOVEME: v3.0.0 */.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it would help to remove it together with the code that was deprecated. Another option would be to include toHaveWarned in the code which would fail when deprecation gets removed. Unless I missed something.

@@ -8,7 +8,6 @@ export { decodeEntities };
export { keycodes };

export * from './blob-cache';
export * from './deprecation';
Copy link
Member

Choose a reason for hiding this comment

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

Love the effort to rid the world of ./utils bit by bit ❤️

@ellatrix ellatrix added [Type] Code Quality Issues or PRs that relate to code quality and removed [Type] Code Quality Issues or PRs that relate to code quality labels Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants