Data Module: Add isFulfilled API for advanced resolver use cases#6084
Data Module: Add isFulfilled API for advanced resolver use cases#6084youknowriad merged 3 commits intomasterfrom
Conversation
aduth
left a comment
There was a problem hiding this comment.
Curious whether you have any thoughts on just making this a property of the resolver function. Seems like it's a bit more intuitive to migrate a function to having more advanced isFulfilled behavior, as it's just adding a property instead of reshaping as an object:
function myResolver( state, ...args ) {
// do something.
}
myResolver.isFulfilled = ( state, ...args ) => {
// returns a boolean.
};
data/index.js
Outdated
| const store = stores[ reducerKey ]; | ||
| const resolver = isPlainObject( newResolvers[ key ] ) ? newResolvers[ key ] : { fulfill: newResolvers[ key ] }; | ||
|
|
||
| const rawFullfill = async ( ...args ) => { |
data/index.js
Outdated
| const fulfill = memoize( async ( ...args ) => { | ||
| const store = stores[ reducerKey ]; | ||
| const store = stores[ reducerKey ]; | ||
| const resolver = isPlainObject( newResolvers[ key ] ) ? newResolvers[ key ] : { fulfill: newResolvers[ key ] }; |
There was a problem hiding this comment.
Personal preference: Since we're only re-shaping this if not already a plain object, seems we'd only need a condition for that, not if/else, avoiding repetition of property access:
let resolver = newResolvers[ key ];
if ( ! isPlainObject( resolver ) ) {
resolver = { fulfill: resolver };
}
That was an option I considered and honestly I don't feel strongly either way. I opted for the object approach in anticipation for future potential properties (caching invalidation etc...), I feel the object scales better instead of assigning several keys. |
d620056 to
4a9803d
Compare
There was a problem hiding this comment.
Looks good.
I think I agree with @youknowriad that the object approach is the way to go. It looks more straightforward.
|
I wonder if we really need to be opinionated on it, or if we just check for the property whether it's an object or other: let resolver = newResolvers[ key ];
if ( ! resolver ) {
return selector;
}
if ( ! resolver.fulfill ) {
resolver = { fulfill: resolver };
}I'm fine with as-is too. |
569851c to
286a028
Compare
|
Update the PR to be less opinionated about it :) |
|
Edit: Please ignore, I had a typo in Previous commentFound this while looking through the code to search exactly for something like this. I guess we should document this somewhere?Unfortunately it doesn't seem to work with generator functions: // This works.
export function* getProfile() {
yield something();
}// This does not.
export const getProfile = {
*fullfill() {
yield something();
},
isFulfilled( state ) {
return ! isEmpty( state.profile.data );
},
};This is the error I'm seeing: Can someone confirm that I'm not doing something wrong? |
In some advanced use cases, the memoization of the resolvers is not enough to avoid similar requests to be triggered multiple times:
To solve this, I'm introducing an
isFulfilledAPI to the resolvers.We can now write resolvers like this:
This resolver will be triggered each time we call the selector unless
isFulfilledreturns true.Testing instructions
Nothing is changing for now, I'm planning to use this in #5875