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

alshakero · 2020-01-19T17:24:01Z

Hello,

Absolutely loving Gutenberg. Thank you so much!

Currently, if you use select('core').getEntityRecords(...) to get records, and you issue an update against these records using saveEntityRecord (I suspect any update effect will surface this bug), you'll get unstable intermediate states until the final state settles.

See GIF (all data here is mocked):

This is because calling saveEntityRecord with an update does the following:

Calls getEntityRecord to fetch the current persisted state of the entity record
Calls receiveEntityRecords with the new up-to-date state to render the updates
Sends an API fetch with the update patch to persist the update
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 propagated and thus momentarily rendered on the UI, causing a flickering effect, that jumps pack when pointing 2 takes its turn.

This PR proposes removing the call to getEntityRecord, and instead, to optimistically call receiveEntityRecords with the local up-to-date state of the entity record. This fixes the flickering issue.

Changes to tests

saveEntityRecord used to select and use getEntityRecord. getEntityRecord is itself a selector. saveEntityRecord no longer selects nor uses getEntityRecord. This implies that saveEntityRecord yields two SELECT actions short from its previous implementation. This PR proposes removing the expectation of these two SELECTs in the tests.

How has this been tested?

I tested this manually and by running the Gutenberg's tests. By manually I mean I built the core-data package after this change and used it with a local instance of Wordpress, the issue disappeared.

Types of changes

Bugfix, but potentially a breaking change, since saveEntityRecord now yields a different sequence of actions. However, all indirectly concerned JS tests are passing except the affected ones, which I updated according to the new yields.

Edit 💡 : Some E2E tests are failing, I'm looking into it. I would really appreciate some input on this one. It seems way deeper than my current understanding of the flow.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've updated all React Native files affected by any refactorings/renamings in this PR. I couldn't find an *.native.js files that are related to this change.

Thanks.

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.

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.

youknowriad · 2020-01-20T08:51:07Z

packages/core-data/src/actions.js

+
+			// 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 );


I know we had a lot of back and forth here and this is a very sensitive code to change (as you can see from the e2e tests). cc @epiqueras

Ha, I knew I'll break things by this change.

I must have written this in the PR comment. I don't think my patch solves the problem well. But I think it does pin it down; the current version is publishing the outdated state when it shouldn't; this is a bug and it's shows on the UI whenever you update the state.

Would you like me to setup a demo repo?

It's breaking because you are optimistically updating the record to become the subset of edited properties. We need the persistedRecord spread at the beginning of that object. receiveEntityRecords sets the object, it doesn't merge properties to what's already in the store.

We also need currentEdits for later logic, so we can't remove that code. getEntityRecordEdits shouldn't be a problem here.

The issue you are experiencing is happening because getEntityRecord triggers a resolver the first time it fires for a given set of parameters. This resolver fetches and sets the record. You usually don't call saveEntityRecord before having called getEntityRecord, but in this case, you are probably calling getEntityRecords and then calling saveEntityRecord directly.

I think this is something we should fix here if anything because of the performance drag of that unnecessary extra request.

To solve this, we need to change that instance of getEntityRecord to one that does not trigger its resolver. I.e. an alias with a different name that calls it in its implementation, getEntityRecordNoResolver(...) => getEntityRecord(...) or something like that.

We could also look at exposing an API for calling a selector without triggering its resolvers.

Makes perfect sense. Can I introduce this new selector with its docs and such?

That would be great, yes. It's fine to do it in this PR if you add an update to the description. It would be great to add a unit test for it as well; "saveEntityRecord() does not trigger the getEntityRecord resolver."

Thanks for catching this!

Done. But instead of testing that saveEntityRecord doesn't issue an API request (kinda like proving a negative) I tested that it selects getEntityRecordNoResolver instead of getEntityRecord.

Testing that it doesn't issue an API request will also be confusing to someone not fimilar with the history in my opinion. I'm open to changing it though.

I'll update the PR's message in the next couple hours.

To be entirely correct, we should probably have an integration test for the store where we use hasStartedResolution to verify the resolver is not called or even better, an E2E test to reproduce the exact issue you experienced.

But we don't have that setup, and it would be kind of out of scope for this PR.

Proving a negative is fine if the intent is correct. For example, without proving a negative here, if in the future the registry API is changed so that nested selector calls also trigger resolvers, this would break silently.

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)

packages/core-data/src/actions.js

packages/core-data/src/selectors.js

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

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

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

alshakero · 2020-01-20T21:44:18Z

Addressed all

youknowriad · 2020-01-21T11:52:06Z

packages/core-data/src/selectors.js

+ *
+ * @return {Object?} Record.
+ */
+export function getEntityRecordNoResolver( state, kind, name, key ) {


It bothers me that we're introducing this as a new API. For me resolvers are internal and shouldn't be known to public APIs

By nature of the fact that there will always be cases where you want to run a selector locally/without resolvers, resolvers will always have to be somewhat "public."

We should have a registry.selectLocal or something like that, where the returned selectors don't trigger their resolvers.

By nature of the fact that there will always be cases where you want to run a selector locally/without resolvers

Why? The concept of resolvers is meant to be entirely transparent to the Component author.
I'd argue that maybe it's needed for internal usage (inside the same store) but not as a public API of that store.

Authors might also need to call selectors without triggering their resolvers, E.G., to get cached values and compare them with the new ones, to avoid API calls when there is no network, to keep local changes for things that could have been modified by other clients, etc.

I'm not certain about that. The whole idea of the selectors/actions is that the component just asks for data and doesn't need to know whether it's async or sync (close to GraphQL approach of things).

I'm not certain about that. The whole idea of the selectors/actions is that the component just asks for data and doesn't need to know whether it's async or sync (close to GraphQL approach of things).

GraphQL clients expose functions to run queries on the cache.

something which could apply to any selector that has associated resolver

That's what I suggested with a selectLocal or selectCached.

Or maybe we need to avoid triggering resolvers as a result of any control being yielded from an action.

That would force a lot of code duplication between controls and resolvers.

In fact, I wonder if that change alone would be fine, and just omit the last argument from the selector. The documentation generator won't consider the resolver, but I expect the data package will happily pass along all arguments anyways.

Yes, that would also work, albeit it's a bit more manual than just having a selectCached. And would we leave it up to each resolver to decide what to do with the option or would we bail before even calling the resolver?

Should we at least mark this s experimental before the beta release?

I found this issue when reviewing and debugging another instance of this issue 🙂

It seems to me that the root cause is that getEntityRecords (plural) fetches the records from network and stores them in local cache, but if I call getEntityRecord (singular) immediately after that, it's a cache miss. And fetches the record again. That shouldn't be happening. The getEntityRecord selector is effectively resolved, the cache contains up-to-date data and a new request shouldn't be fired. A GraphQL cache wouldn't behave this way.

Yeah, I explained that further up. The problem is that this resolver setup doesn't have a trivial way to define relationships between resolvers like this. I guess we would need a new API like getEntityRecord.shouldResolve = ( action, ...args ) => ....

alshakero added 2 commits January 19, 2020 17:22

Core-data: update tests to match saveEntityRecord yeilded actions

581c16c

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.

alshakero requested review from nerrad and youknowriad as code owners January 19, 2020 17:24

youknowriad reviewed Jan 20, 2020

View reviewed changes

Core-data: Introduce getEntityRecordNoResolver selector

a267c35

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)

alshakero requested review from ajitbohra, chrisvanpatten and mkaz as code owners January 20, 2020 16:50

epiqueras requested changes Jan 20, 2020

View reviewed changes

Address review comments at #19752:

ad785c8

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

epiqueras approved these changes Jan 21, 2020

View reviewed changes

epiqueras merged commit 26062c8 into WordPress:master Jan 21, 2020

youknowriad reviewed Jan 21, 2020

View reviewed changes

epiqueras added this to the Gutenberg 7.4 milestone Feb 3, 2020

epiqueras mentioned this pull request Feb 5, 2020

Core Data: Mark the getEntityRecordNoResolver selector as experimental. #20053

Merged

chriskmnds mentioned this pull request May 8, 2020

[@wordpress/core-data] getEntityRecords and saveEntityRecord don't sync data correctly #22127

Closed

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

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

Uh oh!

Conversation

alshakero commented Jan 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes to tests

How has this been tested?

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alshakero Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alshakero Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alshakero Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alshakero commented Jan 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alshakero commented Jan 19, 2020 •

edited

Loading

alshakero Jan 20, 2020 •

edited

Loading

alshakero Jan 20, 2020 •

edited

Loading

alshakero Jan 20, 2020 •

edited

Loading