Core-data: do not publish outdated state to subscribers during updates (#19752)

alshakero · epiqueras · commit 26062c8ab6b1 · 2020-01-21T06:50:54.000-05:00
* Core-data: do not publish outdated state to subscribers during updates Calling `saveEntityRecord` with an update does the following: 1. Calls `getEntityRecord` to fetch the current persisted state of the entity record 2. Calls `receiveEntityRecords` with the new up-to-date state to render the updates 3. Sends an API fetch with the update patch to persist the update 4. Calls `receiveEntityRecords` again with the new up-to-date *persisted* state The issue here, is that point 1 (Calling `getEntityRecord`) not only fetches the persisted state, but it also internally calls `receiveEntityRecords` itself . This results in the persisted outdated server state to be rendered on the UI, causing a flickering effect, that jumps pack when point 2 takes turn. This commit removes the call to getEntityRecord, and instead, it just calls receiveEntityRecords with the local up-to-date state of the entity record. This fixes the flickering issue. * Core-data: update tests to match saveEntityRecord yeilded actions Given `saveEntityRecord` no longer selects `getEntityRecord`, which itself triggers a SELECT action, two SELECTs are no longer yielded. This commit removes the expectation of these two SELECTs. * Core-data: Introduce getEntityRecordNoResolver selector To allow saveEntityRecord access the latest local full version of an entity without issung an API request. This prevents propogating outdating states to subscribers when saveEntityRecord is called. See: #19752 (comment) * Address review comments at #19752: 1. Capitalize alll added comment messages 2. Alias getEntityRecord with getEntityRecordNoResolver instead of copying it 3. Use describe.each instaed of looping manually in selectors tests
diff --git a/docs/designers-developers/developers/data/data-core.md b/docs/designers-developers/developers/data/data-core.md
@@ -202,6 +202,21 @@ _Returns_
 
 -   `?Object`: The entity record's non transient edits.
 
+<a name="getEntityRecordNoResolver" href="#getEntityRecordNoResolver">#</a> **getEntityRecordNoResolver**
+
+Returns the Entity's record object by key. Doesn't trigger a resolver nor requests the entity from the API if the entity record isn't available in the local state.
+
+_Parameters_
+
+-   _state_ `Object`: State tree
+-   _kind_ `string`: Entity kind.
+-   _name_ `string`: Entity name.
+-   _key_ `number`: Record's key
+
+_Returns_
+
+-   `?Object`: Record.
+
 <a name="getEntityRecords" href="#getEntityRecords">#</a> **getEntityRecords**
 
 Returns the Entity's records.
diff --git a/packages/core-data/README.md b/packages/core-data/README.md
@@ -415,6 +415,21 @@ _Returns_
 
 -   `?Object`: The entity record's non transient edits.
 
+<a name="getEntityRecordNoResolver" href="#getEntityRecordNoResolver">#</a> **getEntityRecordNoResolver**
+
+Returns the Entity's record object by key. Doesn't trigger a resolver nor requests the entity from the API if the entity record isn't available in the local state.
+
+_Parameters_
+
+-   _state_ `Object`: State tree
+-   _kind_ `string`: Entity kind.
+-   _name_ `string`: Entity name.
+-   _key_ `number`: Record's key
+
+_Returns_
+
+-   `?Object`: Record.
+
 <a name="getEntityRecords" href="#getEntityRecords">#</a> **getEntityRecords**
 
 Returns the Entity's records.
diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js
@@ -357,11 +357,10 @@ export function* saveEntityRecord(
 				}
 			}
 
-			// We perform an optimistic update here to clear all the edits that
-			// will be persisted so that if the server filters them, the new
-			// filtered values are always accepted.
+			// Get the full local version of the record before the update,
+			// to merge it with the edits and then propagate it to subscribers
 			persistedEntity = yield select(
-				'getEntityRecord',
+				'getEntityRecordNoResolver',
 				kind,
 				name,
 				recordId
diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js
@@ -107,6 +107,20 @@ export function getEntityRecord( state, kind, name, key ) {
 	return get( state.entities.data, [ kind, name, 'queriedData', 'items', key ] );
 }
 
+/**
+ * Returns the Entity's record object by key. Doesn't trigger a resolver nor requests the entity from the API if the entity record isn't available in the local state.
+ *
+ * @param {Object} state  State tree
+ * @param {string} kind   Entity kind.
+ * @param {string} name   Entity name.
+ * @param {number} key    Record's key
+ *
+ * @return {Object?} Record.
+ */
+export function getEntityRecordNoResolver( state, kind, name, key ) {
+	return getEntityRecord( state, kind, name, key );
+}
+
 /**
  * Returns the entity's record object by key,
  * with its attributes mapped to their raw values.
diff --git a/packages/core-data/src/test/actions.js b/packages/core-data/src/test/actions.js
@@ -41,8 +41,11 @@ describe( 'saveEntityRecord', () => {
 		expect( fulfillment.next( entities ).value.type ).toBe(
 			'SAVE_ENTITY_RECORD_START'
 		);
+
+		// Should select getEntityRecordNoResolver selector (as opposed to getEntityRecord)
+		// see https://github.com/WordPress/gutenberg/pull/19752#discussion_r368498318.
 		expect( fulfillment.next().value.type ).toBe( 'SELECT' );
-		expect( fulfillment.next().value.type ).toBe( 'SELECT' );
+		expect( fulfillment.next().value.selectorName ).toBe( 'getEntityRecordNoResolver' );
 		expect( fulfillment.next().value.type ).toBe( 'SELECT' );
 		expect( fulfillment.next().value.type ).toBe( 'RECEIVE_ITEMS' );
 		const { value: apiFetchAction } = fulfillment.next( {} );
diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js
@@ -8,6 +8,7 @@ import deepFreeze from 'deep-freeze';
  */
 import {
 	getEntityRecord,
+	getEntityRecordNoResolver,
 	getEntityRecords,
 	getEntityRecordChangesByRecord,
 	getEntityRecordNonTransientEdits,
@@ -20,7 +21,11 @@ import {
 	getReferenceByDistinctEdits,
 } from '../selectors';
 
-describe( 'getEntityRecord', () => {
+// getEntityRecord and getEntityRecordNoResolver selectors share the same tests
+describe.each( [
+	[ getEntityRecord ],
+	[ getEntityRecordNoResolver ],
+] )( '%p', ( selector ) => {
 	it( 'should return undefined for unknown record’s key', () => {
 		const state = deepFreeze( {
 			entities: {
@@ -36,7 +41,7 @@ describe( 'getEntityRecord', () => {
 				},
 			},
 		} );
-		expect( getEntityRecord( state, 'root', 'postType', 'post' ) ).toBe( undefined );
+		expect( selector( state, 'root', 'postType', 'post' ) ).toBe( undefined );
 	} );
 
 	it( 'should return a record by key', () => {
@@ -56,7 +61,9 @@ describe( 'getEntityRecord', () => {
 				},
 			},
 		} );
-		expect( getEntityRecord( state, 'root', 'postType', 'post' ) ).toEqual( { slug: 'post' } );
+		expect( selector( state, 'root', 'postType', 'post' ) ).toEqual( {
+			slug: 'post',
+		} );
 	} );
 } );
 

Original file line number	Diff line number	Diff line change
`@@ -357,11 +357,10 @@ export function* saveEntityRecord(`
`357`	`357`	`}`
`358`	`358`	`}`
`359`	`359`
`360`		`- // We perform an optimistic update here to clear all the edits that`
`361`		`- // will be persisted so that if the server filters them, the new`
`362`		`- // filtered values are always accepted.`
	`360`	`+ // Get the full local version of the record before the update,`
	`361`	`+ // to merge it with the edits and then propagate it to subscribers`
`363`	`362`	`persistedEntity = yield select(`
`364`		`- 'getEntityRecord',`
	`363`	`+ 'getEntityRecordNoResolver',`
`365`	`364`	`kind,`
`366`	`365`	`name,`
`367`	`366`	`recordId`