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

Core Data: Bring back support for nested _fields values #25083

Merged
merged 3 commits into from
Sep 9, 2020

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Sep 4, 2020

Description

Fixes #25078 — The change to getEntityRecords's use of _fields has broken nested meta field requests. In the API, you can pass nested fields for meta, ex: wp-json/wp/v2/posts?_fields=title,meta.demo,id, and this used to work for the getEntityRecords request.

It seems the change in #19498 introduced this:

if ( ! item.hasOwnProperty( field ) ) {
return null;
}

Which is returning null, because it's testing for item['meta.key'], rather than item.meta.key. This PR switches to using the lodash functions has, get, and set, which support string keys like that. I've seen some moves away from lodash, but it seems like the most straightforward fix here.

How has this been tested?

There's a new test case introduced in packages/core-data/src/queried-data/test/selectors.js which catches this case.

You can also test with the code snippet in #25078

Types of changes

Bug fix (non-breaking change which fixes an issue)

@ryelle ryelle added the [Package] Core data /packages/core-data label Sep 4, 2020
@ryelle ryelle requested review from mcsf and youknowriad September 4, 2020 18:04
@ryelle ryelle requested a review from nerrad as a code owner September 4, 2020 18:04
@ryelle ryelle self-assigned this Sep 4, 2020
@github-actions
Copy link

github-actions bot commented Sep 4, 2020

Size Change: +19 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/core-data/index.js 12.3 kB +19 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.41 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 8.5 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/index.js 128 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.64 kB 0 B
build/block-library/editor.css 8.64 kB 0 B
build/block-library/index.js 138 kB 0 B
build/block-library/style-rtl.css 7.6 kB 0 B
build/block-library/style.css 7.6 kB 0 B
build/block-library/theme-rtl.css 754 B 0 B
build/block-library/theme.css 754 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.7 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.55 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/index.js 11.7 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-post/index.js 305 kB 0 B
build/edit-post/style-rtl.css 6.26 kB 0 B
build/edit-post/style.css 6.25 kB 0 B
build/edit-site/index.js 17.1 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 12 kB 0 B
build/edit-widgets/style-rtl.css 2.46 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.6 kB 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 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.57 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@@ -76,12 +77,12 @@ function getQueriedItemsUncached( state, query ) {
// This accounts for the fact that queried items are stored by
// stable key without an associated fields query. Other requests
// may have included fewer fields properties.
const field = fields[ f ];
if ( ! item.hasOwnProperty( field ) ) {
const field = fields[ f ].split( '.' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this value is left as a string, it matches an undefined value in the state— in my testing I had both meta: { key: 'value' } and 'meta.key': undefined on item, and get matches the string key first, returning undefined. I'm not sure where that other key comes from, but preventing that would be a better fix for this workaround.

@youknowriad
Copy link
Contributor

In the API, you can pass nested fields for meta, ex: wp-json/wp/v2/posts?_fields=title,meta.demo,id, and this used to work for the getEntityRecords request.

Just noting that before #25078 doing this would create subtle bugs where some components could receive "incomplete" entities data. Basically _fields was not considered as supported at all. It was more of a coincidence that it "appeared" to be working. That said, I'm not against adding support for nested fields into getEntityRecords.

@iandunn
Copy link
Member

iandunn commented Sep 7, 2020

I tested the PR and it fixed the problem for me 👍🏻

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.

Thanks

@ryelle ryelle merged commit a84d673 into master Sep 9, 2020
@ryelle ryelle deleted the fix/get-entity-meta-fields branch September 9, 2020 16:33
@github-actions github-actions bot added this to the Gutenberg 9.0 milestone Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Core data /packages/core-data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Meta fields undefined after reuse commit
3 participants