From 90f5adcc03af30b7ec9f9ad061279e194c6cb8ef Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Sun, 19 Jan 2020 17:22:19 +0100 Subject: [PATCH 1/4] 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. --- packages/core-data/src/actions.js | 38 ++++++++++++++++++------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index 3a08117082084..7382d0ae65643 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -357,28 +357,34 @@ 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. - persistedEntity = yield select( - 'getEntityRecord', - kind, - name, - recordId - ); - currentEdits = yield select( - 'getEntityRecordEdits', - kind, - name, - recordId - ); - yield receiveEntityRecords( kind, name, { ...persistedEntity, ...data }, undefined, true ); + /* + // the code below issues an API fetch action against the server and gets the outdates pre-modification state of the record + // and since getEntityRecord calls receiveEntityRecords, this outdated state is rendered to the user, causing an unstable (flickering) + // intermediate UI state + + //////// disabled ////////// + // 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. + persistedEntity = yield select( + 'getEntityRecord', + kind, + name, + recordId + ); + //////// disabled ////////// + */ + + // optimistic update subscribers (eg trigger re-renders) with the modifications applied to the record + // and on their way to be persisted on the server + yield receiveEntityRecords( kind, name, { ...data }, undefined, true ); updatedRecord = yield apiFetch( { path, method: recordId ? 'PUT' : 'POST', data, } ); + yield receiveEntityRecords( kind, name, updatedRecord, undefined, true ); } } catch ( _error ) { From 581c16c503e3efc2b69b9d18acac7b502e70c2cc Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Sun, 19 Jan 2020 18:11:58 +0100 Subject: [PATCH 2/4] 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. --- packages/core-data/src/test/actions.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/core-data/src/test/actions.js b/packages/core-data/src/test/actions.js index ffcee0efbca50..e1be14deabc40 100644 --- a/packages/core-data/src/test/actions.js +++ b/packages/core-data/src/test/actions.js @@ -42,8 +42,6 @@ describe( 'saveEntityRecord', () => { 'SAVE_ENTITY_RECORD_START' ); expect( fulfillment.next().value.type ).toBe( 'SELECT' ); - expect( fulfillment.next().value.type ).toBe( 'SELECT' ); - expect( fulfillment.next().value.type ).toBe( 'SELECT' ); expect( fulfillment.next().value.type ).toBe( 'RECEIVE_ITEMS' ); const { value: apiFetchAction } = fulfillment.next( {} ); expect( apiFetchAction.request ).toEqual( { @@ -78,8 +76,6 @@ describe( 'saveEntityRecord', () => { 'SAVE_ENTITY_RECORD_START' ); expect( fulfillment.next().value.type ).toBe( 'SELECT' ); - expect( fulfillment.next().value.type ).toBe( 'SELECT' ); - expect( fulfillment.next().value.type ).toBe( 'SELECT' ); expect( fulfillment.next().value.type ).toBe( 'RECEIVE_ITEMS' ); const { value: apiFetchAction } = fulfillment.next( {} ); expect( apiFetchAction.request ).toEqual( { @@ -104,8 +100,6 @@ describe( 'saveEntityRecord', () => { 'SAVE_ENTITY_RECORD_START' ); expect( fulfillment.next().value.type ).toBe( 'SELECT' ); - expect( fulfillment.next().value.type ).toBe( 'SELECT' ); - expect( fulfillment.next().value.type ).toBe( 'SELECT' ); expect( fulfillment.next().value.type ).toBe( 'RECEIVE_ITEMS' ); const { value: apiFetchAction } = fulfillment.next( {} ); expect( apiFetchAction.request ).toEqual( { From a267c35384be7a62fd80ceda9f5413cde150a435 Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Mon, 20 Jan 2020 17:50:19 +0100 Subject: [PATCH 3/4] 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: https://github.com/WordPress/gutenberg/pull/19752#discussion_r368498318 --- .../developers/data/data-core.md | 15 +++++ packages/core-data/README.md | 15 +++++ packages/core-data/src/actions.js | 36 +++++------- packages/core-data/src/selectors.js | 14 +++++ packages/core-data/src/test/actions.js | 11 ++++ packages/core-data/src/test/selectors.js | 55 ++++++++++--------- 6 files changed, 100 insertions(+), 46 deletions(-) diff --git a/docs/designers-developers/developers/data/data-core.md b/docs/designers-developers/developers/data/data-core.md index 8b1f9a41e3378..3d310092669ca 100644 --- 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. +# **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. + # **getEntityRecords** Returns the Entity's records. diff --git a/packages/core-data/README.md b/packages/core-data/README.md index c1e1c27691fe8..ca399d641821d 100644 --- 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. +# **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. + # **getEntityRecords** Returns the Entity's records. diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index 7382d0ae65643..24be4031390a7 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -357,27 +357,21 @@ export function* saveEntityRecord( } } - /* - // the code below issues an API fetch action against the server and gets the outdates pre-modification state of the record - // and since getEntityRecord calls receiveEntityRecords, this outdated state is rendered to the user, causing an unstable (flickering) - // intermediate UI state - - //////// disabled ////////// - // 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. - persistedEntity = yield select( - 'getEntityRecord', - kind, - name, - recordId - ); - //////// disabled ////////// - */ - - // optimistic update subscribers (eg trigger re-renders) with the modifications applied to the record - // and on their way to be persisted on the server - yield receiveEntityRecords( kind, name, { ...data }, undefined, true ); + // 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( + 'getEntityRecordNoResolver', + kind, + name, + recordId + ); + currentEdits = yield select( + 'getEntityRecordEdits', + kind, + name, + recordId + ); + yield receiveEntityRecords( kind, name, { ...persistedEntity, ...data }, undefined, true ); updatedRecord = yield apiFetch( { path, diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js index a4ea47ac8c745..6169b230aca03 100644 --- 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 get( state.entities.data, [ kind, name, 'queriedData', 'items', 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 index e1be14deabc40..90343168f33a7 100644 --- a/packages/core-data/src/test/actions.js +++ b/packages/core-data/src/test/actions.js @@ -41,7 +41,14 @@ 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.selectorName ).toBe( 'getEntityRecordNoResolver' ); + expect( fulfillment.next().value.type ).toBe( 'SELECT' ); + expect( fulfillment.next().value.type ).toBe( 'RECEIVE_ITEMS' ); const { value: apiFetchAction } = fulfillment.next( {} ); expect( apiFetchAction.request ).toEqual( { @@ -76,6 +83,8 @@ describe( 'saveEntityRecord', () => { 'SAVE_ENTITY_RECORD_START' ); expect( fulfillment.next().value.type ).toBe( 'SELECT' ); + expect( fulfillment.next().value.type ).toBe( 'SELECT' ); + expect( fulfillment.next().value.type ).toBe( 'SELECT' ); expect( fulfillment.next().value.type ).toBe( 'RECEIVE_ITEMS' ); const { value: apiFetchAction } = fulfillment.next( {} ); expect( apiFetchAction.request ).toEqual( { @@ -100,6 +109,8 @@ describe( 'saveEntityRecord', () => { 'SAVE_ENTITY_RECORD_START' ); expect( fulfillment.next().value.type ).toBe( 'SELECT' ); + expect( fulfillment.next().value.type ).toBe( 'SELECT' ); + expect( fulfillment.next().value.type ).toBe( 'SELECT' ); expect( fulfillment.next().value.type ).toBe( 'RECEIVE_ITEMS' ); const { value: apiFetchAction } = fulfillment.next( {} ); expect( apiFetchAction.request ).toEqual( { diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index ae75557d40f1a..011af8288d52e 100644 --- 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,43 +21,47 @@ import { getReferenceByDistinctEdits, } from '../selectors'; -describe( 'getEntityRecord', () => { - it( 'should return undefined for unknown record’s key', () => { - const state = deepFreeze( { - entities: { - data: { - root: { - postType: { - queriedData: { - items: {}, - queries: {}, +// getEntityRecord and getEntityRecordNoResolver selectors share the same tests +Object.entries( { getEntityRecord, getEntityRecordNoResolver } ).forEach( ( [ name, func ] ) => { + // the interpolation is needed due to https://github.com/jest-community/eslint-plugin-jest/issues/203 + describe( `${ name }`, () => { + it( 'should return undefined for unknown record’s key', () => { + const state = deepFreeze( { + entities: { + data: { + root: { + postType: { + queriedData: { + items: {}, + queries: {}, + }, }, }, }, }, - }, + } ); + expect( func( state, 'root', 'postType', 'post' ) ).toBe( undefined ); } ); - expect( getEntityRecord( state, 'root', 'postType', 'post' ) ).toBe( undefined ); - } ); - it( 'should return a record by key', () => { - const state = deepFreeze( { - entities: { - data: { - root: { - postType: { - queriedData: { - items: { - post: { slug: 'post' }, + it( 'should return a record by key', () => { + const state = deepFreeze( { + entities: { + data: { + root: { + postType: { + queriedData: { + items: { + post: { slug: 'post' }, + }, + queries: {}, }, - queries: {}, }, }, }, }, - }, + } ); + expect( func( state, 'root', 'postType', 'post' ) ).toEqual( { slug: 'post' } ); } ); - expect( getEntityRecord( state, 'root', 'postType', 'post' ) ).toEqual( { slug: 'post' } ); } ); } ); From ad785c85f1b7b41915b7db547a2fab52d28478b4 Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Mon, 20 Jan 2020 22:39:35 +0100 Subject: [PATCH 4/4] 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 --- packages/core-data/src/actions.js | 3 +- packages/core-data/src/selectors.js | 2 +- packages/core-data/src/test/actions.js | 6 +-- packages/core-data/src/test/selectors.js | 58 ++++++++++++------------ 4 files changed, 34 insertions(+), 35 deletions(-) diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index 24be4031390a7..681d4000998f6 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -357,7 +357,7 @@ export function* saveEntityRecord( } } - // get the full local version of the record before the update, + // 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( 'getEntityRecordNoResolver', @@ -378,7 +378,6 @@ export function* saveEntityRecord( method: recordId ? 'PUT' : 'POST', data, } ); - yield receiveEntityRecords( kind, name, updatedRecord, undefined, true ); } } catch ( _error ) { diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js index 6169b230aca03..495ebbbf94d79 100644 --- a/packages/core-data/src/selectors.js +++ b/packages/core-data/src/selectors.js @@ -118,7 +118,7 @@ export function getEntityRecord( state, kind, name, key ) { * @return {Object?} Record. */ export function getEntityRecordNoResolver( state, kind, name, key ) { - return get( state.entities.data, [ kind, name, 'queriedData', 'items', key ] ); + return getEntityRecord( state, kind, name, key ); } /** diff --git a/packages/core-data/src/test/actions.js b/packages/core-data/src/test/actions.js index 90343168f33a7..23e9463b03ebb 100644 --- a/packages/core-data/src/test/actions.js +++ b/packages/core-data/src/test/actions.js @@ -42,13 +42,11 @@ describe( 'saveEntityRecord', () => { 'SAVE_ENTITY_RECORD_START' ); - // should select getEntityRecordNoResolver selector (as opposed to getEntityRecord) - // see https://github.com/WordPress/gutenberg/pull/19752#discussion_r368498318 + // 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.selectorName ).toBe( 'getEntityRecordNoResolver' ); - expect( fulfillment.next().value.type ).toBe( 'SELECT' ); - expect( fulfillment.next().value.type ).toBe( 'RECEIVE_ITEMS' ); const { value: apiFetchAction } = fulfillment.next( {} ); expect( apiFetchAction.request ).toEqual( { diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index 011af8288d52e..07a2540d6dd4e 100644 --- a/packages/core-data/src/test/selectors.js +++ b/packages/core-data/src/test/selectors.js @@ -22,45 +22,47 @@ import { } from '../selectors'; // getEntityRecord and getEntityRecordNoResolver selectors share the same tests -Object.entries( { getEntityRecord, getEntityRecordNoResolver } ).forEach( ( [ name, func ] ) => { - // the interpolation is needed due to https://github.com/jest-community/eslint-plugin-jest/issues/203 - describe( `${ name }`, () => { - it( 'should return undefined for unknown record’s key', () => { - const state = deepFreeze( { - entities: { - data: { - root: { - postType: { - queriedData: { - items: {}, - queries: {}, - }, +describe.each( [ + [ getEntityRecord ], + [ getEntityRecordNoResolver ], +] )( '%p', ( selector ) => { + it( 'should return undefined for unknown record’s key', () => { + const state = deepFreeze( { + entities: { + data: { + root: { + postType: { + queriedData: { + items: {}, + queries: {}, }, }, }, }, - } ); - expect( func( state, 'root', 'postType', 'post' ) ).toBe( undefined ); + }, } ); + expect( selector( state, 'root', 'postType', 'post' ) ).toBe( undefined ); + } ); - it( 'should return a record by key', () => { - const state = deepFreeze( { - entities: { - data: { - root: { - postType: { - queriedData: { - items: { - post: { slug: 'post' }, - }, - queries: {}, + it( 'should return a record by key', () => { + const state = deepFreeze( { + entities: { + data: { + root: { + postType: { + queriedData: { + items: { + post: { slug: 'post' }, }, + queries: {}, }, }, }, }, - } ); - expect( func( state, 'root', 'postType', 'post' ) ).toEqual( { slug: 'post' } ); + }, + } ); + expect( selector( state, 'root', 'postType', 'post' ) ).toEqual( { + slug: 'post', } ); } ); } );