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: Remove deprecations slated for 3.7 removal #9163

Merged
merged 11 commits into from Aug 29, 2018

Conversation

Projects
None yet
5 participants
@youknowriad
Contributor

youknowriad commented Aug 20, 2018

This PR removes deprecations scheduled to be removed in the upcoming 3.7.0 release.

  • wp.components.withAPIData has been removed. Please use the Core Data module or wp.apiFetch directly instead.
  • wp.data.dispatch("core").receiveTerms has been deprecated. Please use wp.data.dispatch("core").receiveEntityRecords instead.
  • getCategories resolvers has been deprecated. Please use getEntityRecords resolver instead.
  • wp.data.select("core").getTerms has been deprecated. Please use wp.data.select("core").getEntityRecords instead.
  • wp.data.select("core").getCategories has been deprecated. Please use wp.data.select("core").getEntityRecords instead.
  • wp.data.select("core").isRequestingTerms has been deprecated. Please use wp.data.select("core").getEntitiesByKind instead.
  • wp.data.restrictPersistence, wp.data.setPersistenceStorage and wp.data.setupPersistence has been removed. Please use the data persistence plugin instead.

Since we removed withApiData, we don't need wp.api.init anymore. And some inline scripts as well.

  • update changelogs
@@ -25,9 +25,6 @@ module.exports = {
env: {
'jest/globals': true,
},
globals: {
wpApiSettings: true,

This comment has been minimized.

@aduth

aduth Aug 20, 2018

Member

Aside: I was excited for a moment that we'd removed our last global. Then I saw we still have wp in eslint/config.js. Looking at remaining usage, I'm inclined to think we should just update those remaining few to at least point to window.wp so we can remove / discourage any other use of the global.

@aduth

aduth Aug 20, 2018

Member

Aside: I was excited for a moment that we'd removed our last global. Then I saw we still have wp in eslint/config.js. Looking at remaining usage, I'm inclined to think we should just update those remaining few to at least point to window.wp so we can remove / discourage any other use of the global.

This comment has been minimized.

@youknowriad

youknowriad Aug 20, 2018

Contributor

Actually, it looks like there's no direct wp global usage anymore.

Edit: There is but it's not being caught by eslint when I removed the config

@youknowriad

youknowriad Aug 20, 2018

Contributor

Actually, it looks like there's no direct wp global usage anymore.

Edit: There is but it's not being caught by eslint when I removed the config

This comment has been minimized.

@aduth

aduth Aug 20, 2018

Member

I still see some wp.oldEditor and wp.codeEditor?

@aduth

aduth Aug 20, 2018

Member

I still see some wp.oldEditor and wp.codeEditor?

This comment has been minimized.

@youknowriad

youknowriad Aug 20, 2018

Contributor

yes, not sure why eslint is not catching them when I remove the global from eslint/config.js

@youknowriad

youknowriad Aug 20, 2018

Contributor

yes, not sure why eslint is not catching them when I remove the global from eslint/config.js

This comment has been minimized.

@youknowriad

youknowriad Aug 20, 2018

Contributor

Notice that I removed the global from the config and the test is still passing. Any idea why?

@youknowriad

youknowriad Aug 20, 2018

Contributor

Notice that I removed the global from the config and the test is still passing. Any idea why?

This comment has been minimized.

@aduth

aduth Aug 20, 2018

Member

I think it's eslint-config-wordpress:

https://github.com/WordPress-Coding-Standards/eslint-config-wordpress/blob/73a8d0a67ef714159b2df2aa16ce4d9180162761/index.js#L13

The false value is a bit misleading. It means that the variable is considered a global, but it's not allowed to be replaced:

https://eslint.org/docs/user-guide/configuring#specifying-globals

@aduth

aduth Aug 20, 2018

Member

I think it's eslint-config-wordpress:

https://github.com/WordPress-Coding-Standards/eslint-config-wordpress/blob/73a8d0a67ef714159b2df2aa16ce4d9180162761/index.js#L13

The false value is a bit misleading. It means that the variable is considered a global, but it's not allowed to be replaced:

https://eslint.org/docs/user-guide/configuring#specifying-globals

This comment has been minimized.

@aduth

aduth Aug 20, 2018

Member

The behavior of false is a bit frustrating as well because it makes it unclear how a project like ours could override the default config to disallow the global. Maybe worth discussing in tomorrow's JS chat?

@aduth

aduth Aug 20, 2018

Member

The behavior of false is a bit frustrating as well because it makes it unclear how a project like ours could override the default config to disallow the global. Maybe worth discussing in tomorrow's JS chat?

This comment has been minimized.

@youknowriad

youknowriad Aug 20, 2018

Contributor

We already said we'd provide a new eslint package in Gutenberg with the updated guidelines discussed in previous meetings. We can remove these globals entirely from this new package.

We can discuss removing the globals altogether (as part of our new JS coding standards for ESnext)

@youknowriad

youknowriad Aug 20, 2018

Contributor

We already said we'd provide a new eslint package in Gutenberg with the updated guidelines discussed in previous meetings. We can remove these globals entirely from this new package.

We can discuss removing the globals altogether (as part of our new JS coding standards for ESnext)

This comment has been minimized.

@aduth

aduth Aug 20, 2018

Member

We already said we'd provide a new eslint package in Gutenberg with the updated guidelines discussed in previous meetings.

It's still an open question as to whether wp would be a global we want to include in the default configuration, assuming a plugin might expect to use those globals rather than import from npm packages.

@aduth

aduth Aug 20, 2018

Member

We already said we'd provide a new eslint package in Gutenberg with the updated guidelines discussed in previous meetings.

It's still an open question as to whether wp would be a global we want to include in the default configuration, assuming a plugin might expect to use those globals rather than import from npm packages.

This comment has been minimized.

@youknowriad

youknowriad Aug 20, 2018

Contributor

That's true, maybe we should keep it and just override it in Gutenberg (if possible).

@youknowriad

youknowriad Aug 20, 2018

Contributor

That's true, maybe we should keep it and just override it in Gutenberg (if possible).

@@ -1413,10 +1365,8 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
( function() {
var editorSettings = %s;
window._wpLoadGutenbergEditor = new Promise( function( resolve ) {
wp.api.init().then( function() {

This comment has been minimized.

@youknowriad

youknowriad Aug 20, 2018

Contributor

Pretty happy about this one :)

@youknowriad

youknowriad Aug 20, 2018

Contributor

Pretty happy about this one :)

This comment has been minimized.

@aduth

aduth Aug 20, 2018

Member

Next to eliminate domReady and the promise altogether 😉

@aduth

aduth Aug 20, 2018

Member

Next to eliminate domReady and the promise altogether 😉

@aduth

The Categories block apparently still uses some selectors from core-data and errors with these changes.

Show outdated Hide outdated packages/components/CHANGELOG.md
*
* @param {string} url Site url.
*/
export function unstable__setSiteURL( url ) { // eslint-disable-line camelcase

This comment has been minimized.

@nosolosw

nosolosw Aug 20, 2018

Member

Just a thought: I wish there was an easy way to package the unstable pieces in one place, ala python's __future__ module. The more I think about that, the more convinced I am using the unstable__* prefix is the right trade-off, though.

Publishing a @wordpress/unstable package could lead to some potential conflicts due the dependance on other @wordpress packages to implement the unstable features. Publishing @wordpress/components-unstable for every package we maintain is a logistic burden (and suffers from the same problems than @wordpress/unstable approach). Package those APIs in subpaths such as components/unstable, data/unstable, etc would expose them at the root level path anyway.

@nosolosw

nosolosw Aug 20, 2018

Member

Just a thought: I wish there was an easy way to package the unstable pieces in one place, ala python's __future__ module. The more I think about that, the more convinced I am using the unstable__* prefix is the right trade-off, though.

Publishing a @wordpress/unstable package could lead to some potential conflicts due the dependance on other @wordpress packages to implement the unstable features. Publishing @wordpress/components-unstable for every package we maintain is a logistic burden (and suffers from the same problems than @wordpress/unstable approach). Package those APIs in subpaths such as components/unstable, data/unstable, etc would expose them at the root level path anyway.

This comment has been minimized.

@aduth

aduth Aug 20, 2018

Member

Just a thought: I wish there was an easy way to package the unstable pieces in one place, ala python's __future__ module.

What would be the proposed advantage of this pattern?

@aduth

aduth Aug 20, 2018

Member

Just a thought: I wish there was an easy way to package the unstable pieces in one place, ala python's __future__ module.

What would be the proposed advantage of this pattern?

This comment has been minimized.

@nosolosw

nosolosw Aug 20, 2018

Member

To centralize the unstable APIs in a way that's clearly communicated and maintained. At the moment, we are using different name patterns (unstable__SomeThing, unstableSomething, and can see how in the future someone could also add unstable_SomeThing) which doesn't scale well.

@nosolosw

nosolosw Aug 20, 2018

Member

To centralize the unstable APIs in a way that's clearly communicated and maintained. At the moment, we are using different name patterns (unstable__SomeThing, unstableSomething, and can see how in the future someone could also add unstable_SomeThing) which doesn't scale well.

This comment has been minimized.

This comment has been minimized.

@nosolosw

nosolosw Aug 20, 2018

Member

Has already happened! 😅

@nosolosw

nosolosw Aug 20, 2018

Member

Has already happened! 😅

Updated, it should be ready.

@nosolosw

This comment has been minimized.

Show comment
Hide comment
@nosolosw

nosolosw Aug 24, 2018

Member

Left a few comments about typos. Would it be cool for me to push directly to solve them next time? Don't want to mess with your local changes, but sometimes it feels more work to left the comment that fixing it myself.

Tested a few things that involve categories, and also the code editor. Unsure if there's anything else I should test. Let me know if you want any thing specific tested. Other than that this seems ready to ship.

Member

nosolosw commented Aug 24, 2018

Left a few comments about typos. Would it be cool for me to push directly to solve them next time? Don't want to mess with your local changes, but sometimes it feels more work to left the comment that fixing it myself.

Tested a few things that involve categories, and also the code editor. Unsure if there's anything else I should test. Let me know if you want any thing specific tested. Other than that this seems ready to ship.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 24, 2018

Contributor

Yes, feel free to make changes, I don't min ;). I already updated though :)
And yeah, just testing quickly that the categories block work as expected should be enough.

Contributor

youknowriad commented Aug 24, 2018

Yes, feel free to make changes, I don't min ;). I already updated though :)
And yeah, just testing quickly that the categories block work as expected should be enough.

@gziolo

Super cool to see so many files removed 👍

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 27, 2018

Contributor

I updated the changelog files to match our guidelines, I borrowed the "Internal" and "Polish" titles for some items in the changelog from Babel like suggested by @gziolo

Contributor

youknowriad commented Aug 27, 2018

I updated the changelog files to match our guidelines, I borrowed the "Internal" and "Polish" titles for some items in the changelog from Babel like suggested by @gziolo

@@ -23,7 +23,6 @@
"dependencies": {
"@babel/runtime-corejs2": "7.0.0-beta.56",
"@wordpress/compose": "file:../compose",
"@wordpress/deprecated": "file:../deprecated",

This comment has been minimized.

@gziolo

gziolo Aug 28, 2018

Member

package-lock.json needs to be updated.

@gziolo

gziolo Aug 28, 2018

Member

package-lock.json needs to be updated.

@gziolo

gziolo approved these changes Aug 28, 2018

I didn't discover any issues when testing changes introduced in this PR.

Code changes look good. It's great to see all those obsolete code removed 💯

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

@youknowriad youknowriad merged commit 80ac625 into master Aug 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the remove/deprecated-features-37 branch Aug 29, 2018

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 29, 2018

Member

🎉

Member

gziolo commented Aug 29, 2018

🎉

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Sep 1, 2018

@aduth Do you have link for wp.apiFetch replacement for wp.components.withAPIData? Trying it now and I can get it to work. Thanks!

phpbits commented Sep 1, 2018

@aduth Do you have link for wp.apiFetch replacement for wp.components.withAPIData? Trying it now and I can get it to work. Thanks!

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 6, 2018

Member

@phpbits There is no provided replacement, as the intent with removing withAPIData was to eliminate a strong dependency on REST API, rather expressing data requirements through selectors on the core-data module.

For what it's worth, since it's published as an npm package, withAPIData will forever be available on previous published versions:

Member

aduth commented Sep 6, 2018

@phpbits There is no provided replacement, as the intent with removing withAPIData was to eliminate a strong dependency on REST API, rather expressing data requirements through selectors on the core-data module.

For what it's worth, since it's published as an npm package, withAPIData will forever be available on previous published versions:

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Sep 7, 2018

@aduth Thanks for the response. I've decided to use withSelect since it's the easiest as of the moment.

phpbits commented Sep 7, 2018

@aduth Thanks for the response. I've decided to use withSelect since it's the easiest as of the moment.

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