-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
createResolversCacheMiddleware: remove dependency on core/data store #54733
Conversation
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.
A nice refactoring and the changes look good to me ✅
P.S. It was easier to review with hidden whitespaces - https://github.com/WordPress/gutenberg/pull/54733/files?diff=unified&w=1.
Size Change: -377 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
Flaky tests detected in e52a253. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6297000535
|
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.
Great catch! 💯
LGTM 🚀
4420eeb
to
e52a253
Compare
Hmm, it appears that the failing tests might be related 🤔 |
There were e2e failures that prevent me from merging. I rebased the PR onto the latest |
// If the value is "finished" or "error" it means this resolver has finished its resolution which means we need | ||
// to invalidate it, if it's true it means it's inflight and the invalidation is not necessary. | ||
if ( | ||
( value.status !== 'finished' && |
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.
Judging by the (previously) failing tests, it looks like those optional chaining instances were necessary.
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.
Yes, thanks for the hint! The value
can indeed be undefined
, although our code always sets it to a valid object value. It's a bug in EquivalentKeyMap
where map.delete
merely sets a map entry value to undefined
:
and then the map.forEach
method iterates also through these deleted values, not only through valid ones.
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.
Thanks for the fix, looks good and tests well 👍 🚀
The
core/data
store is a legacy store that provides access to "resolution state" of other stores' resolvers, and exists only to provide backward compatibility. The supported way to access store's "resolution state" is to use its metadata selectors likehasFinishedResolution
.This PR removes the
core/data
dependency from the resolvers cache middleware, which is used by all stores, and select directly from related stores instead.I'm also changing the code slightly to reduce one level of indentation, and removing unneeded optional chaining when looking at
value.status
.How to test:
Sufficiently covered by existing unit tests.