Framework: Add queried-data state helper utility #6395

aduth · 2018-04-24T18:17:45Z

Related: #6180
Related: #6360

This pull request seeks to work toward a generalized solution for queried / paginated data resources. It is implemented as an independent (but currently bundled) set of state helpers, including reducer, selectors, and action creators for operating on queried data. Currently, only the categories data has been refactored, and it does not currently take advantage of pagination (as this is not used by the Categories block). Further, as discovered in #6360, some additional work is required for memoization of "same" (but not strictly equal) query fulfillment.

The hope is that each resource (core-data state key) can simply reuse the existing helpers from queried-data. For terms, this looks like:

export const terms = onSubKey( 'taxonomy' )( queriedDataReducer );

Some additional helpers may be required to expand this out to other state like media, particularly for ensuring that the reducer only operates on relevant queried data. For terms this is handled automatically through the onSubKey higher-order reducer creator, where state is only tracked for actions which include a taxonomy property. Likewise, we may create other higher-order reducers which test for the presence of some resource-specific marker, to be applied as an extension of the baseline receiveQueriedItems action creator (e.g. ifActionMatches( ( action ) => action.queried === 'media' )).

The state shape is normalized, with queries tracked as stable keys, both to avoid storing redundant data, and to avoid duplicate tracking for equivalent queries:

Example:

{
	"terms": {
		"categories": {
			"items": {
				"1": {
					"id": 1,
					"name": "Uncategorized"
				},
				"2": {
					"id": 2,
					"name": "Cats"
				}
			},
			"queries": {
				"s=cat": {
					"itemIds": [ 1, 2 ],
					"requestingPageByPerPage": {}
				},
				"s=cats": {
					"itemIds": [ 2 ],
					"requestingPageByPerPage": {}
				}
			}
		}
	}
}

Testing instructions:

This should only impact the Categories block, which is the sole consumer of categories data. Verify that there are no regressions in the display of categories block, including the "Loading" placeholder and ultimately the display of categories.

Ensure unit tests pass:

npm test

youknowriad · 2018-05-01T20:05:00Z

core-data/queried-data/reducer.js

+
+	// Since query is optional for handled actions, avoid tracking empty object
+	// for default query on initialization.
+	withReturnUndefinedOnUnhandledDefault,


While these Higher Order Reducers are very clever, I wonder if they are too clever for maintainability :).

withReturnUndefinedOnUnhandledDefault seems very Java to me (aka I have no idea what it does in reading it).

Is it better or worse than com.seriouscompany.business.java.fizzbuzz.packagenamingpackage.interfaces.strategies. SystemOutFizzBuzzOutputStrategy ? 😄

I ended up removing withReturnUndefinedOnUnhandledDefault. It was needed because while ifMatchesAction is meant to allow the reducer to be run conditionally, we must still handle the initialization case. The other option is to provide a default value as an argument of ifMatchesAction, but this felt redundant when the reducer should be able to handle this on its own. Instead, in 623bad6 I remove support for optional query from getQueryParts and test for the property in the action for transforming via replaceAction. Thus, we get the same end-result.

youknowriad · 2018-05-01T20:11:20Z

core-data/queried-data/reducer.js

+			perPage
+		);
+	},
+	requestingPageByPerPage( state = {}, action ) {


Do you think it's better to track the requesting state closely to the data or better in a separate "network requests" reducer. I've used mainly the first option for a long time but lately, I tend to think that the second one might be better as it's more generic and also speaks to the fact that these requests are "optional" in an offline mode.

Do you think it's better to track the requesting state closely to the data or better in a separate "network requests" reducer. I've used mainly the first option for a long time but lately, I tend to think that the second one might be better as it's more generic and also speaks to the fact that these requests are "optional" in an offline mode.

I thought about this a bit and while I tend to agree that requests doesn't need to be tied so deeply to the items / queries, in practice if we try to separate them we'd end up duplicating much of the same logic anyways. As implemented, they're at least separate so far as being their own individual reducers (earlier iterations were combined into one). The behavior of getQueryParts relies on the fact that we generate a stable key which does not include pagination details. To track separately, we'd need a stable key with the pagination information. I see a few options here:

Manually concatenate page and perPage back into the stable key string in requests state

Replace getQueryParts with a simpler getStableKey, and for cases where we need to omit pagination details, do so before passing the object to getStableKey

Avoid building in pagination awareness into queried-data altogether. Just treat each query as distinct from one another, even on pagination arguments.

On reflection, the last of these is tempting, and could simplify things overall. The idea with the current implementation is that queries are considered the same ignoring pagination information.

There's also the question of: How much does "queried-data" make sense as a thing separate from the REST API? In your offline optional requests system you have in mind, would we still have concepts of querying by ?order=desc&page=2 ?

youknowriad · 2018-05-01T20:17:53Z

core-data/queried-data/selectors.js

+ *
+ * @return {?Array} Query items.
+ */
+export function getQueriedItems( state, query ) {


Do you think this should be memoized to avoid creating a new array on each call?

One issue with memoization is the same as described at #6360 (comment) . For example, consider:

export default withSelect( ( select ) => { const { getCategories } = select( 'core' ); return { categories: getCategories( { order: 'desc' } ), }; } )( CategoriesBlock );

In this case, we'd encounter a memory leak in that each object reference would be strictly non-equal.

The getQueryParts function gets around this by using the withWeakMapCache helper, which is designed to alleviate this by caching only as long as the object reference is in scope, otherwise allowing garbage collection to occur. Unfortunately that's only effective where we have a single object argument, not two.

We could perhaps keep a local WeakMap cache in scope, and do memoization manually.

Can we have a specific memoization here where we compare the first argument using strict equality (state) and the second one using deep equality (using EquivalentKeyMap maybe)?

Can we have a specific memoization here where we compare the first argument using strict equality (state) and the second one using deep equality (using EquivalentKeyMap maybe)?

Was able to get this idea working in c502dfc.

youknowriad · 2018-05-01T20:19:53Z

core-data/queried-data/reducer.js

+/**
+ * Higher-order reducer which returns undefined if the original reducer returns
+ * the default state value. Used in combination with `onSubKey` in avoiding
+ * assingment for undefined value to key.


I had some trouble to understand the exact purpuse of this HoR even with the description here. I think it's the equivalent of a "pure" Higher Order Reducer right?

I had some trouble to understand the exact purpuse of this HoR even with the description here. I think it's the equivalent of a "pure" Higher Order Reducer right?

While it's since been removed, the original purpose was not for purity, but rather to avoid the case where in state initialization, the reducer would be run, and since query was optional for getQueryParts, the initial state would end up looking like:

{ "items": {}, "queries": { "": { "itemIds": null, "requestingPageByPerPage": {}, } } }

...Where it would mistake the initialization as the default (empty) query.

youknowriad · 2018-05-01T20:21:31Z

core-data/queried-data/constant.js

+ *
+ * @type {string}
+ */
+export const TOGGLE_IS_REQUESTING = '@@queried-data/TOGGLE_IS_REQUESTING';


Any specific reason to introduce the constants. Not that I'm strongly against them but I do wonder about having some actions defined as constant and others not.

Any specific reason to introduce the constants. Not that I'm strongly against them but I do wonder about having some actions defined as constant and others not.

It was implemented in the mindset of being an independent module utility, with prefixes on the action types to avoid potential conflicts. In earlier iterations, I had the consumer listening for the particular action types, where I didn't want them to be concerned with said prefixes. Since now only the internal reducer is concerned, we could just replace them with strings. Ultimately, I don't consider these files to be part of core-data or the data module patterns whatsoever, so the conventions don't overlap as much as it might appear on the surface.

Maybe a bit premature to modularize this, but having reimplemented effectively the same thing on many separate occasions, I was motivated toward something more universal 😅

danielbachhuber

I've tested this locally to confirm category UI still loads as expected. However, I can't comment to whether the abstraction is useful, though. It doesn't appear to address #5820

youknowriad

LGTM 👍 My comments are not really blocking, mostly questions

Simplifies reducer to avoid handling case where reducer is called on undefined initialization, not with query.

aduth · 2018-05-04T17:38:52Z

Also, to make sure there weren't any large incompatibilities, I took a stab at what it'd take to port entities to using this approach. I think it'll be fairly straight forward, and the following patch gets most of the way there:

https://gist.github.com/aduth/0c69a1ee43cf07cbfaa0316e64186f2b

I'll not include this as part of this pull request, but as a proof-of-concept in anticipation of a follow-up.

aduth · 2018-05-04T17:51:51Z

Deliberating more on whether to flatten this into core-data or not, and to treat queries as distinct (including pagination), I recall expecting that queries will have more metadata that is intended to be shared across all pages of data. For example, total items. This was included in early iterations here, but dropped since it's not yet used (though I expect it will be needed).

On the point of tying requests state to items queries, at its most abstract the goal of such a tool is in accommodating behavior where only subsets of the total data associated with a given query may be known at a given time. The only thing not currently capable in a barebones implementation of this is satisfying the need to detect whether a resolver is pending. The data module already memoizes resolver fulfillment, so I'm wondering if we could leverage awareness of whether there's a next in the resolver generator and create a generic selector which operates on pending resolver state. That way, we wouldn't need to explicitly track requests at all. Let me see if I can come up with anything here.

youknowriad · 2018-06-12T10:19:19Z

@aduth Hey Andrew, do you plan to revisit this one? That's one of the missing pieces to drop our current withAPIData usage.

aduth · 2018-06-12T17:19:43Z

Yes, I'd like to. I'll need to catch up on where I'd left off, and in what ways more recent changes would have posed conflict (largely #6462 and #6600).

aduth · 2018-07-17T15:17:19Z

I've unfortunately not yet found the availability to prioritize this one recently. If you'd like to pick it up, feel free. Otherwise, I'll plan to revisit once API and general writing flow has stabilized.

youknowriad · 2018-08-01T10:39:30Z

I'm taking a look at refreshing this PR :)

aduth · 2018-08-02T20:13:58Z

Superseded by #8357

aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 24, 2018

aduth requested a review from youknowriad April 24, 2018 18:17

This was referenced Apr 27, 2018

Initial entities implementation #6462

Merged

Switch post-author component to use /wp/v2/users/?who=authors #6515

Merged

youknowriad reviewed May 1, 2018

View reviewed changes

danielbachhuber reviewed May 1, 2018

View reviewed changes

youknowriad approved these changes May 2, 2018

View reviewed changes

aduth added 5 commits May 4, 2018 10:49

Framework: Add queried-data state helper utility

b402e57

Data: Refactor core categories data using queried-data

1e599cb

Data: Avoid tracking itemIds while request in progress

fda0520

Data: Remove default query handling

623bad6

Simplifies reducer to avoid handling case where reducer is called on undefined initialization, not with query.

Data: Add deep caching for queried items

c502dfc

aduth force-pushed the add/core-data-pagination branch from c3d396e to c502dfc Compare May 4, 2018 16:06

This was referenced May 4, 2018

Data: Refactor resolver fulfillment / request progress as data state #6600

Merged

Improve performance; Stop unnecessary rerenders caused by withColors #6686

Merged

aduth mentioned this pull request May 30, 2018

Data Module: Add support for action creators as generators #6999

Closed

youknowriad mentioned this pull request Jun 27, 2018

Post author selector: try downshift #7478

Closed

4 tasks

youknowriad mentioned this pull request Aug 1, 2018

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

Merged

aduth closed this Aug 2, 2018

aduth deleted the add/core-data-pagination branch August 2, 2018 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Add queried-data state helper utility #6395

Framework: Add queried-data state helper utility #6395

aduth commented Apr 24, 2018 •

edited

Loading

youknowriad May 1, 2018

danielbachhuber May 1, 2018

aduth May 1, 2018

aduth May 4, 2018

youknowriad May 1, 2018

aduth May 4, 2018

youknowriad May 1, 2018

aduth May 1, 2018

youknowriad May 1, 2018

aduth May 4, 2018

youknowriad May 1, 2018

aduth May 4, 2018 •

edited

Loading

youknowriad May 1, 2018

aduth May 4, 2018

danielbachhuber left a comment

youknowriad left a comment

aduth commented May 4, 2018

aduth commented May 4, 2018

youknowriad commented Jun 12, 2018

aduth commented Jun 12, 2018

aduth commented Jul 17, 2018

youknowriad commented Aug 1, 2018

aduth commented Aug 2, 2018

Framework: Add queried-data state helper utility #6395

Framework: Add queried-data state helper utility #6395

Conversation

aduth commented Apr 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

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 May 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber left a comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

aduth commented May 4, 2018

aduth commented May 4, 2018

youknowriad commented Jun 12, 2018

aduth commented Jun 12, 2018

aduth commented Jul 17, 2018

youknowriad commented Aug 1, 2018

aduth commented Aug 2, 2018

aduth commented Apr 24, 2018 •

edited

Loading

aduth May 4, 2018 •

edited

Loading