Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core-data: do not publish outdated state to subscribers during updates #19752
Core-data: do not publish outdated state to subscribers during updates #19752
Changes from 3 commits
90f5adc
581c16c
a267c35
ad785c8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GraphQL clients expose functions to run queries on the cache.
That's what I suggested with a
selectLocal
orselectCached
.That would force a lot of code duplication between controls and resolvers.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we at least mark this s experimental before the beta release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#20053
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 callgetEntityRecord
(singular) immediately after that, it's a cache miss. And fetches the record again. That shouldn't be happening. ThegetEntityRecord
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ) => ...
.