Handle resolution errors in @wordpress/data #38669
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adamziel
added
[Package] Data
|
Size Change: +4.7 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
adamziel
force-pushed
the
data/handle-resolution-errors
branch
from
ad0197f
to
71678cc
Compare
|
Haven't reviewed anything yet. Just want to say that you're a superhero, @adamziel 😍 . What would we do without you? |
adamziel
force-pushed
the
data/handle-resolution-errors
branch
from
f1e56e9
to
5c15583
Compare
adamziel
changed the title
youknowriad
reviewed
Feb 14, 2022
Mamaduka
reviewed
Feb 15, 2022
jsnajdr
reviewed
Feb 16, 2022
|
I added a test case for falsy errors: it( 'returns true if the resolution has failed even if the error is falsy', async () => {
registry.registerStore( 'store', {
reducer: ( state = null, action ) => {
if ( action.type === 'RECEIVE' ) {
return action.items;
}
return state;
},
selectors: {
getItems: ( state ) => state,
},
resolvers: {
getItems: () => {
throw null;
},
},
} );
expect(
registry.select( 'store' ).hasResolutionFailed( 'getItems' )
).toBeFalsy();
await resolve( registry, 'getItems' );
expect(
registry.select( 'store' ).hasResolutionFailed( 'getItems' )
).toBeTruthy();
} ); |
adamziel
force-pushed
the
data/handle-resolution-errors
branch
from
0c77c84
to
9bbfda1
Compare
adamziel
requested review from
ajitbohra,
mkaz and
ryanwelcher
as code owners
adamziel
force-pushed
the
data/handle-resolution-errors
branch
from
8ffdc26
to
4a28600
Compare
|
@jsnajdr this one is ready for another review 🙏 |
8 tasks
kevin940726
reviewed
Feb 24, 2022
kevin940726
reviewed
Mar 2, 2022
…s return StateValue
This is the best and it's definitely required. Thank you for working on this 👏 |
kevin940726
approved these changes
Mar 4, 2022
|
Do you see any blockers @youknowriad, or are we good? |
youknowriad
approved these changes
Mar 7, 2022
adamziel
added
the
Needs Dev Note
|
@youknowriad thanks! I added a changelog entry. Other than that I don't think the meta store is documented anywhere. I grepped all the meta selector and meta action names and didn't find any markdown documentation other than the autogenerated ones. I will make sure to include that in the new documents that I'm writing. Oh, and I wasn't sure about the dev note label but I added it anyway – let's figure it out when the time comes. |
37 tasks
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
@wodrpress/dataexposes a resolvers API, but ignores any errors thrown during the resolution:gutenberg/packages/core-data/src/resolvers.js
Lines 107 to 111 in 70730e6
This PR proposes an error handling mechanism that works roughly as follows:
failResolutionaction which also finalizes the resolutionhasLastResolutionFailedandgetLastResolutionFailureselectorsresolveSelectwill now be rejected in case of resolution error. Previously it just hanged forever.To that last point, we may need to wrap any
resolveSelectin try/catch to make sure redux actions don't start unexpectedly throwing exceptions.Test plan
It is a new feature, so testing is mostly about confirming the CI checks, reading the code, and discussing.
cc @youknowriad @jsnajdr @mcsf @gziolo @ellatrix @draganescu @noisysocks @jsnajdr @Mamaduka @jorgefilipecosta @talldan @getdave @kevin940726 @anton-vlasenko