Data Module: Adding The queried data handling into the entities abstraction #8357

youknowriad · 2018-08-01T11:35:41Z

This PR supersedes #6395

I wasn't able to rebase it because of the long history, so I just started fresh and copy/pasted code (added a co-author)

Here are the differences with the original PR:

This integrates the queries data into the entities abstraction
It drops the tracking of the inflight requests in the queried data reducer (we already have the built-in resolvers state in the data module)
It drops the constants for consistency with the way we do in all the modules. We can bring them back if we want to expose the module later, I think we'd probably want to expose the entities data entirely and not only the queried-data part.
I didn't refactor the existing reducers/selectors. These are meant to be deprecated in Remove unused terms, taxonomies and categories code #8250

Testing instructions

Check that the editor loads properly

aduth

Noticing a strange regression on this branch. The Tags panel is displayed twice. I cannot manage Categories:

aduth · 2018-08-02T20:14:48Z

packages/core-data/src/actions.js

@@ -56,13 +61,13 @@ export function addEntities( entities ) {
 * @param {string}       kind    Kind of the received entity.
 * @param {string}       name    Name of the received entity.
 * @param {Array|Object} records Records received.
+ * @param {Object?}      query  Query Object.


Based on documentation, nullable type should be denoted with ? as prefix, not suffix:

http://usejsdoc.org/tags-type.html

Eslint should be more proactive 😄

aduth · 2018-08-02T20:22:43Z

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

+
+/**
+ * Returns items for a given query, or null if the items are not known. Caches
+ * result by both per state (by reference) and per query (by deep equality).


Grammar: Remove first "by" in this line.

aduth · 2018-08-02T20:24:42Z

packages/core-data/src/selectors.js

@@ -148,15 +149,17 @@ export function getEntityRecord( state, kind, name, key ) {
 * @param {Object} state  State tree
 * @param {string} kind   Entity kind.
 * @param {string} name   Entity name.
+ * @param {?Object} query    Optional terms query.


Minor: Alignment on (a) argument name, (b) argument description.

aduth · 2018-08-02T20:31:31Z

Likely related to regression: I'm seeing three separate actions dispatched for RECEIVE_ITEMS of the taxonomy type. The last one only includes "Tags". The first has the full set of taxonomies I'd expect.

gziolo

I left my feedback. This is looking really good. I have some concerns regarding caching strategy. I'm not quite sure what are use cases we are dealing with at the moment. However, I anticipate some issues in the future based on my experience with similar solutions I worked with in the past.

We also should start thinking how to invalidate the cache when data changes on the server and we want re-fetch it because optimistic updates are not sufficient.

gziolo · 2018-08-02T14:50:23Z

packages/core-data/src/queried-data/actions.js

+ *
+ * @return {Object} Action object.
+ */
+export function receiveQueriedItems( query = {}, items ) {


Nit: It looks like the order of the params should be reversed given that query is optional.

gziolo · 2018-08-03T06:45:51Z

packages/core-data/src/actions.js

@@ -56,13 +61,13 @@ export function addEntities( entities ) {
 * @param {string}       kind    Kind of the received entity.
 * @param {string}       name    Name of the received entity.
 * @param {Array|Object} records Records received.
+ * @param {Object?}      query  Query Object.


Eslint should be more proactive 😄

gziolo · 2018-08-03T06:57:22Z

packages/core-data/src/selectors.js

+export function getEntityRecords( state, kind, name, query ) {
+	const queriedState = get( state.entities.data, [ kind, name ] );
+	if ( ! queriedState ) {
+		return [];


Should [] be assigned to const, moved out and always return the same reference to avoid rerenders? I think createSelector assured that in the past.

I think this is a rare use-case that can happen only if the entities has just been registered (lazy entities).

gziolo · 2018-08-03T07:00:19Z

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

 								post: { slug: 'post' },
 								page: { slug: 'page' },
 							},
+							queries: {
+								'': [ 'post', 'page' ],


Is this order of items random or can we predict it?

I think it doesn't matter after looking at implementation details.

gziolo · 2018-08-03T07:03:52Z

packages/core-data/src/utils/test/if-matching-action.js

+	} );
+
+	it( 'should not call reducer if predicate returns false', () => {
+		const state = {};


It would be nice to wrap with deepFreeze / Object.freeze to ensure there is no mutation.

gziolo · 2018-08-03T07:05:32Z

packages/core-data/src/utils/test/if-matching-action.js

+	}
+
+	it( 'should call reducer if predicate returns true', () => {
+		const reducer = createEnhancedReducer( () => true );


Isn't it edge case when state is undefined? There should be another test for an existing state.

I guess it's not important because the idea is just to check that the reducer was called or not, I'm updating the default state anyway :)

gziolo · 2018-08-03T07:08:36Z

packages/core-data/src/utils/test/on-sub-key.js

+	} );
+
+	it( 'should ignore actions where property not present', () => {
+		const state = {};


It would be nice to have initial state wrapped with Object.freeze, too.

gziolo · 2018-08-03T07:17:35Z

packages/core-data/src/queried-data/get-query-parts.js

+		switch ( key ) {
+			case 'page':
+			case 'perPage':
+				parts[ key ] = value;


Should value be mapped to string or number to ensure it has the same type when passing to the cache no matter what plugin developer provided as a value? Knowing the dynamic

it( 'encodes stable string key with page normalized', () => { const first = getQueryParts( { '?': '&', b: 2, page: 1 } ); const second = getQueryParts( { b: 2, '?': '&', page: '1' } ); expect( first ).toEqual( second ); expect( first ).toEqual( { page: 1, perPage: 10, stableKey: '%3F=%26&b=2', } ); } );

This fails.

Not certain about this, I'll defer to @aduth as he originally built this utility.

Yes, it seems reasonable to Number( value ) here.

Should value be mapped to string or number to ensure it has the same type when passing to the cache no matter what plugin developer provided as a value?

Normalized to number as of 37e0d7a.

gziolo · 2018-08-03T07:23:16Z

packages/core-data/src/queried-data/get-query-parts.js

+					( parts.stableKey ? '&' : '' ) +
+					encodeURIComponent( key ) +
+					'=' +
+					encodeURIComponent( value )


Does it assume that value can by only represented by primitive types?

Otherwise, we should add deep sorting for arrays or objects and serialization mechanism.

Example test case:

it( 'encodes stable string key when using arrays', () => { const first = getQueryParts( { '?': '&', b: [ 3, 2 ] } ); const second = getQueryParts( { b: [ 2, 3 ], '?': '&' } ); expect( first ).toEqual( second ); expect( first ).toEqual( { page: 1, perPage: 10, stableKey: '%3F=%26&b=2', } ); } );

Not sure how stable key should look like in such case.

Yeah, currently I think this would only support primitive values. It's not quite so straight-forward, but I could imagine recursing into getQueryParts for non-primitive types.

Does it assume that value can by only represented by primitive types?

Added deep value support (via addQueryArgs) in 0368719.

youknowriad · 2018-08-03T13:03:03Z

Noticing a strange regression on this branch. The Tags panel is displayed twice. I cannot manage Categories:

So the issue was that we were considering a single entity record fetch as a query fetch because we're calling receiveEntityRecords at the end of the resolver with no query argument which was resetting the query items set for the "getEntityRecords" call.

I had two options to fix it:

Consider the getEntityRecord call as a separate fetch with no relationship with queried data, which means using another RECEIVE_ITEM action instead of RECEIVE_ITEMS or just use RECEIVE_ITEMS without a query property.
Consider getEntityRecord as a special "query". For example when fetching the post with ID: 1, set the query object to { ID: 1 }. That's the option I took in 895463b to fix the issue.

It should be fixed now

aduth · 2018-08-03T18:35:27Z

or just use RECEIVE_ITEMS without a query property.

On a glance, this is the exact use-case for why receiveItems exists as a separate action creator in the first place.

More to come as I evaluate more deeply..

aduth · 2018-08-03T19:02:47Z

I pushed 803b12e as my preferred alternative. I think this could be taken further to a separate receiveEntityRecord action creator if we don't want to have to have receiveEntityRecords vary its logic based on the presence of a query.

youknowriad · 2018-08-04T07:34:54Z

@aduth I'm fine either way, I just thought that the fetch one request is just a "special" query but I don't have strong arguments.

…action Co-authored-by: aduth <andrew.duthie@automattic.com>

gziolo

There is one more question that needs to be answered before we proceed: https://github.com/WordPress/gutenberg/pull/8357/files#r207456393. Otherwise, it looks great. Looking forward to seeing it n use 🚀

youknowriad added the [Package] Data /packages/data label Aug 1, 2018

youknowriad self-assigned this Aug 1, 2018

youknowriad requested review from gziolo and aduth August 1, 2018 11:35

youknowriad force-pushed the add/queries-data branch 3 times, most recently from d095c69 to 0408943 Compare August 1, 2018 13:47

youknowriad requested a review from a team August 2, 2018 10:55

aduth mentioned this pull request Aug 2, 2018

Framework: Add queried-data state helper utility #6395

Closed

aduth previously requested changes Aug 2, 2018

View reviewed changes

gziolo reviewed Aug 3, 2018

View reviewed changes

youknowriad and others added 5 commits August 4, 2018 16:32

Data Module: Adding The queried data handling into the entities abstr…

700fcb4

…action Co-authored-by: aduth <andrew.duthie@automattic.com>

Fix a single entity fetch

a78f7fa

Data: Vary received entities action by query presence

f9fffb0

Data: Normalize queried pagination values as numeric

37e0d7a

Data: Support deep value serialization of query key

0368719

aduth force-pushed the add/queries-data branch from 803b12e to 0368719 Compare August 4, 2018 20:50

aduth approved these changes Aug 4, 2018

View reviewed changes

gziolo approved these changes Aug 6, 2018

View reviewed changes

youknowriad merged commit 81bc778 into master Aug 6, 2018

youknowriad deleted the add/queries-data branch August 6, 2018 07:54

aduth mentioned this pull request Aug 21, 2018

[WIP] Try/fresh data as data plugin #8754

Closed

4 tasks

dependabot-preview bot mentioned this pull request Dec 31, 2019

Bump @wordpress/i18n from 2.0.0 to 3.7.0 wearerequired/rest-likes#97

Closed

dependabot-preview bot mentioned this pull request Feb 4, 2020

Bump @wordpress/jest-console from 3.4.0 to 3.5.0 tdreyno/druyan#215

Merged

dependabot-preview bot mentioned this pull request Apr 15, 2020

Bump @wordpress/babel-plugin-makepot from 3.4.0 to 3.5.0 swissspidy/gutenberg-i18n-block#93

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data Module: Adding The queried data handling into the entities abstraction #8357

Data Module: Adding The queried data handling into the entities abstraction #8357

youknowriad commented Aug 1, 2018 •

edited

Loading

aduth left a comment •

edited

Loading

aduth Aug 2, 2018

gziolo Aug 3, 2018

aduth Aug 2, 2018

aduth Aug 2, 2018

aduth commented Aug 2, 2018

gziolo left a comment

gziolo Aug 2, 2018

gziolo Aug 3, 2018

gziolo Aug 3, 2018

youknowriad Aug 6, 2018

gziolo Aug 3, 2018

gziolo Aug 3, 2018

gziolo Aug 3, 2018

gziolo Aug 3, 2018

youknowriad Aug 3, 2018

gziolo Aug 3, 2018

gziolo Aug 3, 2018

gziolo Aug 3, 2018

youknowriad Aug 3, 2018

aduth Aug 3, 2018

aduth Aug 4, 2018

gziolo Aug 3, 2018

gziolo Aug 3, 2018

aduth Aug 3, 2018

aduth Aug 4, 2018

youknowriad commented Aug 3, 2018

aduth commented Aug 3, 2018

aduth commented Aug 3, 2018

youknowriad commented Aug 4, 2018

gziolo left a comment •

edited

Loading

Data Module: Adding The queried data handling into the entities abstraction #8357

Data Module: Adding The queried data handling into the entities abstraction #8357

Conversation

youknowriad commented Aug 1, 2018 • edited Loading

aduth left a comment • 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

aduth commented Aug 2, 2018

gziolo left a 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

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

youknowriad commented Aug 3, 2018

aduth commented Aug 3, 2018

aduth commented Aug 3, 2018

youknowriad commented Aug 4, 2018

gziolo left a comment • edited Loading

Choose a reason for hiding this comment

youknowriad commented Aug 1, 2018 •

edited

Loading

aduth left a comment •

edited

Loading

gziolo left a comment •

edited

Loading