Skip to content
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

Data Module: Add isFulfilled API for advanced resolver use cases #6084

Merged
merged 3 commits into from
Apr 12, 2018

Conversation

youknowriad
Copy link
Contributor

In some advanced use cases, the memoization of the resolvers is not enough to avoid similar requests to be triggered multiple times:

  • When using complex args (new instance of objects)
  • Multiple generators dealing with the same data.

To solve this, I'm introducing an isFulfilled API to the resolvers.

We can now write resolvers like this:

const myResolver: { 
  fulfill: ( state, ...args ) => // do something.
  isFulfilled: ( state, ...args ) => // returns a boolean. 
},

This resolver will be triggered each time we call the selector unless isFulfilled returns true.

Testing instructions

Nothing is changing for now, I'm planning to use this in #5875

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rawFullfill = async ( ...args ) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "Fullfill" -> "Fulfill"

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 ] };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 };
}

@youknowriad
Copy link
Contributor Author

Curious whether you have any thoughts on just making this a property of the resolver function.

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.

@youknowriad youknowriad force-pushed the try/data-module-is-fulfilled branch from d620056 to 4a9803d Compare April 11, 2018 07:59
Copy link
Member

@atimmer atimmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

I think I agree with @youknowriad that the object approach is the way to go. It looks more straightforward.

@aduth
Copy link
Member

aduth commented Apr 11, 2018

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.

@youknowriad youknowriad force-pushed the try/data-module-is-fulfilled branch from 569851c to 286a028 Compare April 12, 2018 08:26
@youknowriad
Copy link
Contributor Author

Update the PR to be less opinionated about it :)

@ocean90
Copy link
Member

ocean90 commented Apr 13, 2019

Edit: Please ignore, I had a typo in fullfill, it's fulfill.

Previous comment Found 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:

Uncaught (in promise) TypeError: resolver.fulfill.apply is not a function
    at _callee2$ (namespace-store.js:348)

Can someone confirm that I'm not doing something wrong?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants