Framework: Remove deprecations slated for 3.7 removal #9163

youknowriad · 2018-08-20T13:12:20Z

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

aduth · 2018-08-20T13:14:42Z

.eslintrc.js

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


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.

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

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

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

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

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

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?

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)

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.

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

youknowriad · 2018-08-20T13:29:39Z

lib/client-assets.php

@@ -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() {


Pretty happy about this one :)

Next to eliminate domReady and the promise altogether 😉

aduth · 2018-08-20T14:18:35Z

components/code-editor/index.js

@@ -11,6 +11,20 @@ import CodeEditor from './editor';
 import Placeholder from '../../packages/components/src/placeholder';
 import Spinner from '../../packages/components/src/spinner';

+/**
+ * Moddule constants


Typo: "Moddule" -> "Module"

This will be wrongly associated with the variable declaration as its JSDoc.,

"Module constants" demarcations are useless and encourage a bad practice (a pattern to avoid meaningful documentation of top-level variables).

aduth

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

aduth · 2018-08-20T14:48:10Z

packages/components/CHANGELOG.md

@@ -0,0 +1,3 @@
+## v3.0.0 (Unreleased)
+
+- `withAPIData` has been removed. Please use the Core Data module or `@wordpress/apiFetch` directly instead.


Correct name is @wordpress/api-fetch

oandregal · 2018-08-20T14:56:48Z

packages/core-data/CHANGELOG.md

+- Breaking: `getCategories` resolvers has been deprecated. Please use `getEntityRecords` resolver instead.
+- Breaking: `select("core").getTerms` has been deprecated. Please use `select("core").getEntityRecords` instead.
+- Breaking: `select("core").getCategories` has been deprecated. Please use `select("core").getEntityRecords` instead.
+- Breaking: `select("core").isRequestingTerms` has been deprecated. Please use `select("core").getEntitiesByKind` instead.


I think select("core").isRequestingCategories is missing here.

oandregal · 2018-08-20T15:16:15Z

components/code-editor/index.js

+ *
+ * @param {string} url Site url.
+ */
+export function unstable__setSiteURL( url ) { // eslint-disable-line camelcase


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.

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?

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.

Don't forget __experimentalSomething

https://github.com/WordPress/gutenberg/blob/master/docs/reference/coding-guidelines.md#experimental-apis

😆

Has already happened! 😅

oandregal · 2018-08-20T15:31:44Z

packages/core-data/CHANGELOG.md

+## v2.0.0 (Unreleased)
+
+- Breaking: `dispatch("core").receiveTerms` has been deprecated. Please use `dispatch("core").receiveEntityRecords` instead.
+- Breaking: `getCategories` resolvers has been deprecated. Please use `getEntityRecords` resolver instead.


We should get rid of the last usages of getCategories and isRequestingCategories.

Also, should say:

Breaking: getCategories resolver ...

Updated, it should be ready.

oandregal · 2018-08-24T09:13:15Z

docs/reference/deprecated.md

@@ -12,7 +12,9 @@ Gutenberg's deprecation policy is intended to support backwards-compatibility fo
 - `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").isRequestingCategories` has been deprecated. Please use `wp.data.select("core/data").isResolving` instead.
 - `wp.data.select("core").isRequestingTerms` has been deprecated. Please use `wp.data.select("core").getEntitiesByKind` instead.


I guess

wp.data.select("core").isRequestingTerms has been deprecated. Please use wp.data.select("core").getEntitiesByKind

should be modified to

wp.data.select("core").isRequestingTerms has been deprecated. Please use wp.data.select("core/data").isResolving

oandregal · 2018-08-24T09:16:34Z

packages/core-data/CHANGELOG.md

+- Breaking: `getCategories` resolvers has been deprecated. Please use `getEntityRecords` resolver instead.
+- Breaking: `select("core").getTerms` has been deprecated. Please use `select("core").getEntityRecords` instead.
+- Breaking: `select("core").getCategories` has been deprecated. Please use `select("core").getEntityRecords` instead.
+- `wp.data.select("core").isRequestingCategories` has been deprecated. Please use `wp.data.select("core/data").isResolving` instead.


Should this say Breaking: wp.data.select ...?

oandregal · 2018-08-24T09:17:25Z

packages/core-data/CHANGELOG.md

+- Breaking: `select("core").getTerms` has been deprecated. Please use `select("core").getEntityRecords` instead.
+- Breaking: `select("core").getCategories` has been deprecated. Please use `select("core").getEntityRecords` instead.
+- `wp.data.select("core").isRequestingCategories` has been deprecated. Please use `wp.data.select("core/data").isResolving` instead.
+- Breaking: `select("core").isRequestingTerms` has been deprecated. Please use `select("core").getEntiisResolvingtiesByKind` instead.


Please use wp.data.select( 'core/data' ).isResolving instead

oandregal · 2018-08-24T09:21:06Z

docs/reference/deprecated.md

@@ -12,7 +12,9 @@ Gutenberg's deprecation policy is intended to support backwards-compatibility fo
 - `getCategories` resolvers has been deprecated. Please use `getEntityRecords` resolver instead.


This should say:

getCategories resolver ...

oandregal · 2018-08-24T09:36:49Z

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 · 2018-08-24T09:38:47Z

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.

aduth · 2018-08-24T13:43:35Z

packages/block-library/src/categories/edit.js

- categories: getCategories(),
- isRequesting: isRequestingCategories(),
+ categories: getEntityRecords( 'taxonomy', 'category', query ),
+ isRequesting: isResolving( 'core', 'getEntityRecords', 'taxonomy', 'category', query ),


Is this the correct usage? Specifically the 'core' argument

gutenberg/packages/core-data/src/selectors.js

Line 28 in 5632f68

function isResolving( selectorName, ...args ) {

yes, this is the correct usage because I'm using the root one. The selector in core/data not the selector in core. I wasn't even aware of the existence of the selector in core.

I think I'm against having this selector in core unless it's made generic (added automatically to all stores)

I think my head is made to spin by the fact that core/data is the reducer key for packages/data and core is the reducer for packages/core-data 😖

Ok then so isn't it still being used wrongly because args is expected as an array, not a spread?

gutenberg/packages/data/src/store/selectors.js

Line 69 in 5632f68

export function isResolving( state, reducerKey, selectorName, args = [] ) {

Yes, we should probably change that. A graceful transition would be to change core/data and alias it with something like data (or something else) and deprecated core/data and then after sometime do the same thing for core (core-data)

True, it's still wrong, It's not the first time either this difference in passing arguments trick me.

gziolo · 2018-08-27T13:05:19Z

packages/core-data/CHANGELOG.md

@@ -0,0 +1,8 @@
+## v2.0.0 (Unreleased)
+
+- Breaking: `dispatch("core").receiveTerms` has been deprecated. Please use `dispatch("core").receiveEntityRecords` instead.


It should be:

## v2.0.0 (Unreleased) ### Breaking Change - `dispatch("core").receiveTerms` has been deprecated. Please use `dispatch("core").receiveEntityRecords` instead. ...

See https://github.com/WordPress/gutenberg/blob/master/CONTRIBUTING.md#maintaining-changelogs.

I copied from another package :)

Just saw that there's a bunch of package that don't follow the guidelines :) I'll update them as well.

Thanks, we merged those guidelines a few days back, so I'm sure we need to be more proactive with enforcing what we agreed on :)

gziolo · 2018-08-27T13:07:29Z

packages/data/CHANGELOG.md

@@ -2,3 +2,4 @@

 - Breaking: The `withRehdyration` function is removed. Use the persistence plugin instead.


The same as above:

## v2.0.0 (Unreleased) ### Breaking Change - The `withRehdyration` function is removed. Use the persistence plugin instead. ...

gziolo

Super cool to see so many files removed 👍

youknowriad · 2018-08-27T13:31:25Z

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

gziolo · 2018-08-28T10:21:56Z

packages/data/package.json

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


package-lock.json needs to be updated.

gziolo

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 · 2018-08-29T19:24:54Z

🎉

phpbits · 2018-09-01T15:58:08Z

@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 · 2018-09-06T17:02:59Z

@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 · 2018-09-07T04:10:02Z

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

youknowriad added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Breaking Change For PRs that introduce a change that will break existing functionality labels Aug 20, 2018

youknowriad self-assigned this Aug 20, 2018

youknowriad requested review from oandregal and aduth August 20, 2018 13:12

aduth reviewed Aug 20, 2018

View reviewed changes

youknowriad commented Aug 20, 2018

View reviewed changes

aduth reviewed Aug 20, 2018

View reviewed changes

aduth previously requested changes Aug 20, 2018

View reviewed changes

oandregal reviewed Aug 20, 2018

View reviewed changes

oandregal reviewed Aug 24, 2018

View reviewed changes

aduth reviewed Aug 24, 2018

View reviewed changes

gziolo reviewed Aug 27, 2018

View reviewed changes

gziolo mentioned this pull request Aug 27, 2018

SDK: Try wp.data with calypso state tree Automattic/wp-calypso#26838

Closed

youknowriad added 5 commits August 28, 2018 10:52

Framework: Remove deprecations slated for 3.7 removal

871e66b

Update the packages changelogs

f998f2f

Remove wp from the allowed globals

bc12383

Remove module constants comment

06813fb

Replace usage of deprecated selectors (getCategories)

7e99890

youknowriad added 4 commits August 28, 2018 10:52

Addding missing deprecation to the docs

e5a5024

Fix typos

9f1afaa

Fix isResolving selector call

dd6253b

Update all changelogs to follow the guidelines

96ff92f

youknowriad force-pushed the remove/deprecated-features-37 branch from 53de1e4 to 96ff92f Compare August 28, 2018 09:53

gziolo reviewed Aug 28, 2018

View reviewed changes

Update the package lock

9b12f66

gziolo approved these changes Aug 28, 2018

View reviewed changes

gziolo added this to the 3.7 milestone Aug 28, 2018

Fix package-lock.json

4571e6c

youknowriad merged commit 80ac625 into master Aug 29, 2018

youknowriad deleted the remove/deprecated-features-37 branch August 29, 2018 16:58

aduth mentioned this pull request Nov 27, 2018

Packages: Extract Eslint config package #7965

Merged

4 tasks

		@@ -0,0 +1,3 @@
		## v3.0.0 (Unreleased)

		- `withAPIData` has been removed. Please use the Core Data module or `@wordpress/apiFetch` directly instead.

		@@ -12,7 +12,9 @@ Gutenberg's deprecation policy is intended to support backwards-compatibility fo
		- `getCategories` resolvers has been deprecated. Please use `getEntityRecords` resolver instead.

		@@ -0,0 +1,8 @@
		## v2.0.0 (Unreleased)

		- Breaking: `dispatch("core").receiveTerms` has been deprecated. Please use `dispatch("core").receiveEntityRecords` instead.

		@@ -2,3 +2,4 @@

		- Breaking: The `withRehdyration` function is removed. Use the persistence plugin instead.

Framework: Remove deprecations slated for 3.7 removal #9163

Framework: Remove deprecations slated for 3.7 removal #9163

Conversation

youknowriad commented Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

youknowriad Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal commented Aug 24, 2018

youknowriad commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

youknowriad commented Aug 27, 2018

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

gziolo commented Aug 29, 2018

phpbits commented Sep 1, 2018 • edited Loading

aduth commented Sep 6, 2018

phpbits commented Sep 7, 2018

youknowriad commented Aug 20, 2018 •

edited

Loading

youknowriad Aug 20, 2018 •

edited

Loading

oandregal Aug 20, 2018 •

edited

Loading

oandregal Aug 20, 2018 •

edited

Loading

oandregal Aug 24, 2018 •

edited

Loading

aduth Aug 24, 2018 •

edited

Loading

gziolo Aug 27, 2018 •

edited

Loading

phpbits commented Sep 1, 2018 •

edited

Loading