Revert introduction of useApiFetch hook and associated changes to Core Navigation block data fetching. #21674

getdave · 2020-04-17T09:14:58Z

This reverts commit f2b6778 which was based on #18669.

# Conflicts:
#	package-lock.json
#	packages/api-fetch/package.json
#	packages/block-library/src/navigation/edit.js

Description

The commit which this PR reverts introduced

(what I believe to be) A regression in the Core Navigation Block data fetching mechanics to prefer apiFetch over Core data entities.
A new hook useApiFetch which unduly promotes the use of apiFetch as a "preferred" API.

The reason this needs to be reverted are:

@aduth feels apiFetch should not be promoted as a first class API for data fetching. Therefore we should not make it easy to use via hooks such as useAPIFetch which was introduced. @aduth suggested the hook should be removed.
The Navigation block's edit implementation was re-written to remove all entity based data fetching via Core Data mechanics in favour of apiFetch. This should be avoided for the reasons I set out here.

You can also read more about why apiFetch might not be a good choice for data fetching.

The only issue with reverting this PR is that we lose the fix to the context parameter which you can read about in the original PR descriptio. We could raise a followup PR to try and restore this functionality if desired.

How has this been tested?

Check Nav Block still works as expected (as it does on master). This revert should cause no regressions.

Types of changes

Breaking change (fix or feature that would cause existing functionality to not work as expected).

This removes a public API in the useApiFetch hook.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

…peEntities` (#18669)" This reverts commit f2b6778. # Conflicts: # package-lock.json # packages/api-fetch/package.json # packages/block-library/src/navigation/edit.js

github-actions · 2020-04-17T09:20:46Z

Size Change: -572 B (0%)

Total Size: 840 kB

Filename	Size	Change
`build/annotations/index.js`	3.62 kB	-1 B
`build/api-fetch/index.js`	3.39 kB	-620 B (18%)	👏
`build/autop/index.js`	2.83 kB	+2 B (0%)
`build/block-directory/index.js`	6.23 kB	-2 B (0%)
`build/block-editor/index.js`	105 kB	+7 B (0%)
`build/block-library/index.js`	112 kB	+62 B (0%)
`build/block-serialization-default-parser/index.js`	1.88 kB	+2 B (0%)
`build/blocks/index.js`	57.7 kB	-4 B (0%)
`build/components/index.js`	198 kB	-13 B (0%)
`build/compose/index.js`	6.65 kB	-5 B (0%)
`build/core-data/index.js`	11.1 kB	+2 B (0%)
`build/edit-navigation/index.js`	3.54 kB	+3 B (0%)
`build/edit-post/index.js`	27.7 kB	+1 B
`build/edit-site/index.js`	10.4 kB	-3 B (0%)
`build/edit-widgets/index.js`	7.46 kB	-4 B (0%)
`build/editor/index.js`	43.3 kB	+2 B (0%)
`build/element/index.js`	4.65 kB	+2 B (0%)
`build/format-library/index.js`	7.31 kB	-2 B (0%)
`build/keyboard-shortcuts/index.js`	2.52 kB	+2 B (0%)
`build/list-reusable-blocks/index.js`	3.12 kB	-2 B (0%)
`build/media-utils/index.js`	5.28 kB	-1 B
`build/nux/index.js`	3.4 kB	+3 B (0%)
`build/rich-text/index.js`	14.8 kB	-3 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.02 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	760 B	0 B
`build/block-editor/style-rtl.css`	10.2 kB	0 B
`build/block-editor/style.css`	10.2 kB	0 B
`build/block-library/editor-rtl.css`	7.09 kB	0 B
`build/block-library/editor.css`	7.09 kB	0 B
`build/block-library/style-rtl.css`	7.17 kB	0 B
`build/block-library/style.css`	7.17 kB	0 B
`build/block-library/theme-rtl.css`	683 B	0 B
`build/block-library/theme.css`	685 B	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/components/style-rtl.css`	16.7 kB	0 B
`build/components/style.css`	16.7 kB	0 B
`build/data-controls/index.js`	1.25 kB	0 B
`build/data/index.js`	8.43 kB	0 B
`build/date/index.js`	5.47 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	569 B	0 B
`build/dom/index.js`	3.1 kB	0 B
`build/edit-navigation/style-rtl.css`	485 B	0 B
`build/edit-navigation/style.css`	485 B	0 B
`build/edit-post/style-rtl.css`	12.3 kB	0 B
`build/edit-post/style.css`	12.3 kB	0 B
`build/edit-site/style-rtl.css`	5.02 kB	0 B
`build/edit-site/style.css`	5.02 kB	0 B
`build/edit-widgets/style-rtl.css`	4.65 kB	0 B
`build/edit-widgets/style.css`	4.64 kB	0 B
`build/editor/editor-styles-rtl.css`	428 B	0 B
`build/editor/editor-styles.css`	431 B	0 B
`build/editor/style-rtl.css`	3.48 kB	0 B
`build/editor/style.css`	3.47 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/style-rtl.css`	502 B	0 B
`build/format-library/style.css`	502 B	0 B
`build/hooks/index.js`	2.13 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.56 kB	0 B
`build/is-shallow-equal/index.js`	710 B	0 B
`build/keycodes/index.js`	1.91 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	226 B	0 B
`build/list-reusable-blocks/style.css`	226 B	0 B
`build/notices/index.js`	1.79 kB	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/plugins/index.js`	2.67 kB	0 B
`build/primitives/index.js`	1.49 kB	0 B
`build/priority-queue/index.js`	788 B	0 B
`build/redux-routine/index.js`	2.84 kB	0 B
`build/server-side-render/index.js`	2.67 kB	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.28 kB	0 B
`build/url/index.js`	4.02 kB	0 B
`build/viewport/index.js`	1.84 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

aduth · 2020-04-17T12:32:10Z

I'm fine with this.

Because useApiFetch shipped in the plugin, can we add a deprecation notice and keep it in for two versions.

And a note here:

https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/developers/backward-compatibility/deprecations.md

Prior art:

https://github.com/WordPress/gutenberg/pull/8082/files#diff-33e0ac9c44b16a617039a563d4c065f3L171

getdave · 2020-04-17T14:38:18Z

@aduth Looking at @wordpress/deprecation and your prior art example, should I be leaving useApiFetch in place within @wordpress/api-fetch and simply wrapping it in a deprecation notice?

aduth · 2020-04-17T16:11:44Z

@aduth Looking at @wordpress/deprecation and your prior art example, should I be leaving useApiFetch in place within @wordpress/api-fetch and simply wrapping it in a deprecation notice?

Leave it in place, yes. deprecation can be called within the hook, probably as one of the first lines of the code.

Normally we'd want to remove any associated documentation as well. In this instance, it turned out fortunate that we neglected to document the new hook, so there's nothing to do there.

youknowriad · 2020-04-17T16:29:10Z

I like this hook personally and I believe if I had to use apiFetch for some reason on a component, it's better to use this hook over a custom lifecycle implementation. Not saying we shouldn't remove it since it's not used elsewhere but 🤷 I wouldn't mind if we replace our adhoc apiFetch calls with it.

(Data store is better in most cases though)

aduth · 2020-04-17T17:08:43Z

Data store is better in most cases though

Inconvenience can be a feature. If we don't think this is an ideal way to implement components, we should go out of our way to avoid introducing conveniences.

More: #18669 (comment)

youknowriad · 2020-04-17T17:11:07Z

I agree with you but If I think as a plugin author, sometimes I have just one off personal endpoints and I don't want to create a whole store for that. In Gutenberg itself it's hard to find a use-cases where a store is not better though (aside the current limitations related to rights)

aduth · 2020-04-17T17:21:08Z

If we had clear and well-respected guidelines around that, it might not be as much a concern. Currently I can't help but see it as being far too ripe for misuse.

Personally, I'd rather spend the effort:

Reducing the perceived burden around "create a whole store for that".
Making behaviors around API fetching more interoperable with other libraries. Let them use something else if they don't want to do it the WordPress way. We don't have to offer a solution which contradicts on our guidelines.

youknowriad · 2020-04-17T17:29:04Z

After using a similar library on a personal project (react-fetching-library), I found that it's perfect for most use-cases and useApiFetch is very similar. So I'm not saying that I'd use it just because it's more convenient than creating a store, I'd use it because the abstraction feels right for these use-cases, a store won't bring anything useful here no matter how easy it is to create.

That said, I'm fine with removing it here right now (it can easily be created on the third-party code itself)

aduth · 2020-04-17T17:34:38Z

Personally, I see no issue to just tell people to use something like react-fetching-library if this is what they want to do.

youknowriad · 2020-04-17T17:44:47Z

Fair, but in that case why do we have an apiFetch package :), they could just use fetch right?

aduth · 2020-04-17T17:54:30Z

This is the part where I relent my absolutism and grant there are balances to make. The value that apiFetch provides in preparing fetch requests for WordPress REST API interaction is something that we need in the preferred way (the data module flow). Beyond that, when I say things like "making behaviors around API fetching more interoperable with other libraries", I'm fully prepared that these may be specific features that we don't strictly need in our own implementations, but exist for the sole purpose of giving plugin authors the choice to integrate with third-party solutions. For me, where the line starts to get drawn is when we offer neatly-packaged solutions that we neither need nor advocate in our own guidelines, and whose existence may cause confusion for continued maintenance within the project in how they contribute to an inconsistent communication of expectations ("when and why it would be appropriate to use one or the other of useAPIFetch vs. core data").

chrisvanpatten · 2020-04-17T18:16:53Z

The only issue with reverting this PR is that we lose the fix to the context parameter which you can read about in the original PR description. We could raise a followup PR to try and restore this functionality if desired.

I think this is a critical question and is a weakness of the current getEntityRecord implementation. I don't know if necessarily it should be required to specify the context manually, or if it should have some way to intelligently determine the context which is available for a given user, but the automatic assumption that a user has (or needs!) edit permissions to view a record is a bit limitation.

getdave · 2020-04-17T19:33:28Z

Reducing the perceived burden around "create a whole store for that".

I've seen apiFetch used a lot in 3rd party blocks. I'd say we should put a big effort into the documentation around the creation of stores otherwise it's usage will proliferate with the aforementioned side effects.

I wonder @aduth would you recommend creating a store for a single block?

That said, I'm fine with removing it here right now (it can easily be created on the third-party code itself)

I'll run with this.

aduth · 2020-04-17T19:36:30Z

I wonder @aduth would you recommend creating a store for a single block?

There's far too many considerations involved in a question like this to be able to give a single concrete answer.

Most cases: Probably not?

getdave · 2020-04-20T08:34:11Z

I've split this PR to make things easier to manage.

One PR to rollback the Nav changes and one PR to deprecate useApiFetch.

This should make things easier to manage and reason about and also to get approving reviews.

Here are the new PRs:

Revert changes to data fetching mechanics of Navigation Block Revert changes to Nav Block data fetching mechanics #21721
Add deprecation notice to useApiFetch hook Adds deprecation notice to useApiFetch hook #21723

See #21674 (comment)

* Adds deprecation notice to useApiFetch hook See #21674 (comment) * Simplify deprecation message * Fix e2e tests by expecting deprecation notice * Fix useApiFetch warning in related tests * Update lockfile * Update warning to use correct deprecation version * Update version ref in tests * Fix remaining versions refs in tests

Revert "Fix Nav Block bug as Contributor user via fix to `*loadPostTy…

d16fe43

…peEntities` (#18669)" This reverts commit f2b6778. # Conflicts: # package-lock.json # packages/api-fetch/package.json # packages/block-library/src/navigation/edit.js

getdave added [Type] Regression Related to a regression in the latest release [Package] Core data /packages/core-data [Package] API fetch /packages/api-fetch [Block] Navigation Affects the Navigation Block labels Apr 17, 2020

getdave requested review from draganescu, youknowriad, noisysocks and aduth April 17, 2020 09:14

getdave requested review from ajitbohra, jorgefilipecosta, mmtr, nerrad, Soean and talldan as code owners April 17, 2020 09:14

getdave self-assigned this Apr 17, 2020

This was referenced Apr 17, 2020

Fix Nav Block bug as Contributor user via fix to *loadPostTypeEntities #18669

Merged

Nav block: enable creation from existing WP Menus #18869

Merged

getdave changed the title ~~Revert "Fix Nav Block bug as Contributor user via fix to *loadPostTypeEntities"~~ Revert introduction of useApiFetch hook and associated changes to Core Navigation block data fetching. Apr 17, 2020

getdave mentioned this pull request Apr 20, 2020

Revert changes to Nav Block data fetching mechanics #21721

Merged

6 tasks

getdave closed this Apr 20, 2020

getdave added a commit that referenced this pull request Apr 20, 2020

Adds deprecation notice to useApiFetch hook

2e7f9ba

See #21674 (comment)

getdave mentioned this pull request Apr 20, 2020

Adds deprecation notice to useApiFetch hook #21723

Merged

6 tasks

getdave added a commit that referenced this pull request Apr 20, 2020

Adds deprecation notice to useApiFetch hook

fce9f96

See #21674 (comment)

getdave mentioned this pull request Apr 22, 2020

Fix e2e tests by removing unecessary assertions & properly installing deps in api-fetch package #21780

Merged

6 tasks

aristath deleted the fix/revert-f2b677851aed870fa8216861561b3ce0197c03c4 branch November 10, 2020 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert introduction of useApiFetch hook and associated changes to Core Navigation block data fetching. #21674

Revert introduction of useApiFetch hook and associated changes to Core Navigation block data fetching. #21674

getdave commented Apr 17, 2020 •

edited

Loading

github-actions bot commented Apr 17, 2020

aduth commented Apr 17, 2020

getdave commented Apr 17, 2020

aduth commented Apr 17, 2020

youknowriad commented Apr 17, 2020 •

edited

Loading

aduth commented Apr 17, 2020 •

edited

Loading

youknowriad commented Apr 17, 2020

aduth commented Apr 17, 2020

youknowriad commented Apr 17, 2020

aduth commented Apr 17, 2020

youknowriad commented Apr 17, 2020 •

edited

Loading

aduth commented Apr 17, 2020 •

edited

Loading

chrisvanpatten commented Apr 17, 2020 •

edited

Loading

getdave commented Apr 17, 2020

aduth commented Apr 17, 2020 •

edited

Loading

getdave commented Apr 20, 2020 •

edited

Loading

Revert introduction of useApiFetch hook and associated changes to Core Navigation block data fetching. #21674

Revert introduction of useApiFetch hook and associated changes to Core Navigation block data fetching. #21674

Conversation

getdave commented Apr 17, 2020 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

github-actions bot commented Apr 17, 2020

aduth commented Apr 17, 2020

getdave commented Apr 17, 2020

aduth commented Apr 17, 2020

youknowriad commented Apr 17, 2020 • edited Loading

aduth commented Apr 17, 2020 • edited Loading

youknowriad commented Apr 17, 2020

aduth commented Apr 17, 2020

youknowriad commented Apr 17, 2020

aduth commented Apr 17, 2020

youknowriad commented Apr 17, 2020 • edited Loading

aduth commented Apr 17, 2020 • edited Loading

chrisvanpatten commented Apr 17, 2020 • edited Loading

getdave commented Apr 17, 2020

aduth commented Apr 17, 2020 • edited Loading

getdave commented Apr 20, 2020 • edited Loading

getdave commented Apr 17, 2020 •

edited

Loading

youknowriad commented Apr 17, 2020 •

edited

Loading

aduth commented Apr 17, 2020 •

edited

Loading

youknowriad commented Apr 17, 2020 •

edited

Loading

aduth commented Apr 17, 2020 •

edited

Loading

chrisvanpatten commented Apr 17, 2020 •

edited

Loading

aduth commented Apr 17, 2020 •

edited

Loading

getdave commented Apr 20, 2020 •

edited

Loading