-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Deprecated: Introduce new module with depreaction util #6914
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
Conversation
packages/devtools/package.json
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me as well, I asked for help: https://wordpress.slack.com/archives/C5UNMSU4R/p1527080592000279.
youknowriad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
|
Re: name, why not just |
22abf4a to
414aad3
Compare
Done 👍 |
414aad3 to
a93d934
Compare
|
This should be good now to merge. |
| { | ||
| selector: 'ImportDeclaration[source.value=/^editor$/]', | ||
| message: 'Use @wordpress/editor as import path instead.', | ||
| selector: 'ImportDeclaration[source.value=/^deprecated/]', |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 :)
| toAsyncIterable, | ||
| } from '../'; | ||
|
|
||
| jest.mock( '@wordpress/utils', () => ( { |
There was a problem hiding this comment.
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 */.
There was a problem hiding this comment.
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.
| export { keycodes }; | ||
|
|
||
| export * from './blob-cache'; | ||
| export * from './deprecation'; |
There was a problem hiding this comment.
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 ❤️
Description
Extracted from #6828.
Related issues: #3955, #6594.
Similar PRs: #6658, #6756, #6758.
This PR introduces new
@wordpress/deprecatedpackage. 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 testTry
wp.deprecated( 'My message' )in the JS console and make sure that deprecation warning is shown properly.Screenshots
Types of changes
Refactoring.
Checklist: