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: Support adding and updating entities #10089

Merged
merged 19 commits into from Oct 27, 2018

Conversation

@youknowriad
Contributor

youknowriad commented Sep 21, 2018

As an example: This PR refactors the HierarchicalTaxonomySelector component to use the data module instead of apiFetch.

This PR is very interesting in several aspects because it shows where we fall shart currently in Async Flow in the Data module and hopefully will allow us to solve this issue.

Notes:

1- First thing to note is that it's not fully functional yet because we need to solve a fundamental problem first: The list of categories is not being refreshed once we add a new category. That's because the queried-data state doest't invalidate the query resolvers/selectors when we add new records.

Options I can think of:

  • Invalidate all the resolvers performing queries. The difficulty is that it's not possible to gather automatically all these resolvers. Not sure there's a technical solution with this approach.

  • Rewrite the queried-data selectors to avoid relying on the IDs returned by the API request but perform the filtering client-side: Doesn't seem great neither because it's not always possible to recreate the filters client side especially since this is extensible by plugins server-side.

Which is to say, I'm not certain how to move forward with this, I need some insights/help @aduth

2- This PR also shows that to create an action composed of two actions:

  • Save category
  • Add the saved category to the categories of the post

we need to know when the first composed action finishes and we need to know its return value (the ID created).

This is solved in this PR by using the return value of the generators. For me it seems an ok compromise and it works well. but I know this can be considered a "bad" practice

@youknowriad youknowriad requested review from aduth and WordPress/gutenberg-core Sep 21, 2018

@youknowriad youknowriad self-assigned this Sep 21, 2018

const taxonomy = getTaxonomy( slug );
const availableTerms = getEntityRecords( 'taxonomy', slug, DEFAULT_QUERY );
const availableTermsTree = buildTermsTree( availableTerms );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Sep 21, 2018

Member

We will be generating a new reference inside buildTermsTree which will probably trigger lost of unnecessary rerenders. As possible solutions, we may create a memoized selector that returns the tree already build. Or create a local memoized one instance of build terms tree, I think something like this may work:

export default compose( [
    (() => {
        const memoizedBuildtermsTree = memize( buildTermsTree, { maxSize: 1 } );
        return withSelect( ( select, { slug } ) => {
            ....
        } );
    })()
@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Sep 21, 2018

Invalidate all the resolvers performing queries. The difficulty is that it's not possible to gather automatically all these resolvers. Not sure there's a technical solution with this approach.

By invaliding, does it mean all resolvers would need to query the server again to retrieve the query?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Sep 21, 2018

By invaliding, does it mean all resolvers would need to query the server again to retrieve the query?

yes, that's the idea, on the next "selector" call of these resolvers, they will fetch again. So, only the components that are still mounted.

@aduth

This comment has been minimized.

Member

aduth commented Sep 21, 2018

Rewrite the queried-data selectors to avoid relying on the IDs returned by the API request but perform the filtering client-side: Doesn't seem great neither because it's not always possible to recreate the filters client side especially since this is extensible by plugins server-side.

See also Query Manager, where this was its second goal to the first we mimicked already by queried-data (normalizing pagination fields). The need to respect plugin extensions is more pronounced than what we were dealing with in Calypso, so it might not be the best option to explore. Will need to think on this more...

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Sep 21, 2018

Would add a parameter to getEntityRecords that allows the query to be updated be an option?
E.g:

getEntityRecords( 'taxonomy', slug, DEFAULT_QUERY, onCategory( previousQueryResult, newCategory ) {
	add new category id to previousQueryResult and sort by count after adding
}

We may then provide helpers, e.g: a dumb update that just adds new elements, a sorted helper that adds sorted by a given property etc...

const taxonomy = getTaxonomy( slug );
const availableTerms = getEntityRecords( 'taxonomy', slug, DEFAULT_QUERY );
const availableTermsTree = buildTermsTree( availableTerms );
return {
hasCreateAction: taxonomy ? get( getCurrentPost(), [ '_links', 'wp:action-create-' + taxonomy.rest_base ], false ) : false,
hasAssignAction: taxonomy ? get( getCurrentPost(), [ '_links', 'wp:action-assign-' + taxonomy.rest_base ], false ) : false,
terms: taxonomy ? select( 'core/editor' ).getEditedPostAttribute( taxonomy.rest_base ) : [],

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Sep 21, 2018

Member

Existing problem but we should have a constant EMPTY_TERMS = [] and use it instead of [] so we avoid generating a new reference.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Sep 21, 2018

@jorgefilipecosta I'm thinking ideally, this is not handled on the caller's side but on the data module side.

A rought proposal:

const resolver = {
  fulfill: () => {},
   shouldInvalidate: ( action, ...args ) => {
      return action.type === 'ADD_ENTITY' && args.name === action.name;
   }
}

Ideally, these invalidation rules can be "composed". If a resolvers ( getEntityRecords ) uses a given action ( receiveItems ) and receiveItems defines that it can be invalidated by addItems then getEntityRecords is also automatically invalidated by addItems

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Sep 21, 2018

I would prefer if we did not have to make a fetch request right after adding new items. I guess a dumb adder, a sorter and a filter by property would cover 99% of the cases and maybe we can generate the updater automatically based on the query that is already being, for complex cases we would invalid and request again.

@aduth

This comment has been minimized.

Member

aduth commented Sep 24, 2018

I would prefer if we did not have to make a fetch request right after adding new items.

Considering that there is nothing about resolvers that necessitates that their implementation incur a network request, I don't know that this is strictly problematic. While an additional implementation could naively destroy the local cache and trigger a follow-up refetch, I could envision some advanced resolver which has its own reconciling cache to more efficiently resolve data after an invalidation.

@aduth

This comment has been minimized.

Member

aduth commented Sep 24, 2018

A rought proposal:

Thought: Would the inverse work / be better? Where an action define (by names?) the selectors it would invalidate?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Sep 24, 2018

Thought: Would the inverse work / be better? Where an action define (by names?) the selectors it would invalidate?

how do you refer to partially applied selectors? say you want to invalidate all calls to a selector with a given first argument.

@aduth

This comment has been minimized.

Member

aduth commented Sep 24, 2018

With that, I guess you could not be granular at all. Still possible, but would require designing selectors to not be so generic. Leaning towards your original proposal. Did you have any worries about it?

@aduth

This comment has been minimized.

Member

aduth commented Sep 24, 2018

I'm more okay with a solution which is "wasteful" in invalidating more than it needs to than one which requires recreating querying in the client, which I feel would be impossible to have be 100% accurate (and thus lead to inconsistencies).

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Sep 26, 2018

I added cache invalidation using the mechanism suggested above #10089 (comment)

This is lacking some unit tests but should be considered for merge soon.

}
case 'INVALIDATE_CACHE': {
const nextState = new EquivalentKeyMap( state );
nextState.set( action.args, undefined );

This comment has been minimized.

@youknowriad

youknowriad Sep 26, 2018

Contributor

@aduth .delete didn't work, probably a bug upstream in EquivalentKeyMap

@mtias mtias referenced this pull request Sep 26, 2018

Closed

wp.data Overview #8354

6 of 6 tasks complete
const isStarting = action.type === 'START_RESOLUTION';
const nextState = new EquivalentKeyMap( state );
nextState.set( action.args, isStarting );
return nextState;
}
case 'INVALIDATE_CACHE': {

This comment has been minimized.

@aduth

aduth Sep 26, 2018

Member

Minor: the data reducer does not hold a cache of data per-se, it only tracks whether resolution has occurred. Therefore, maybe a better name is INVALIDATE_RESOLUTION or INVALIDATE_IS_RESOLVED

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Sep 27, 2018

Thanks @aduth for the updates, I also added some tests and I think this should be ready for a final review.

@youknowriad youknowriad force-pushed the add/update-remove-entities branch from 11bec02 to aea02a9 Oct 22, 2018

@youknowriad youknowriad added this to the 4.2 milestone Oct 22, 2018

@youknowriad youknowriad requested a review from gziolo Oct 22, 2018

@gziolo gziolo added this to In Progress in API freeze via automation Oct 22, 2018

@youknowriad youknowriad force-pushed the add/update-remove-entities branch from 904952e to 5c1fd39 Oct 22, 2018

@youknowriad youknowriad force-pushed the add/update-remove-entities branch from d3799c1 to 0f3780b Oct 27, 2018

@youknowriad youknowriad merged commit c3c8f46 into master Oct 27, 2018

2 checks passed

codecov/project 48.54% (+0.15%) compared to 85d58bb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

API freeze automation moved this from In Progress to Done Oct 27, 2018

@youknowriad youknowriad deleted the add/update-remove-entities branch Oct 27, 2018

@youknowriad youknowriad restored the add/update-remove-entities branch Oct 27, 2018

@youknowriad youknowriad deleted the add/update-remove-entities branch Oct 27, 2018

@TimothyBJacobs

This comment has been minimized.

Contributor

TimothyBJacobs commented Oct 27, 2018

@youknowriad sorry. To clarify, could returning the action value in the withDispatch hoc happen before 5.0? Or is that not happening at all? It’d be extremely helpful for being able to show in the UI when long running actions are occurring.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 27, 2018

@TimothyBJacobs The workaround is to trigger an action setting a flag at the beginning of your async action and unsetting the flag and the end and use a selector to retrieve this flag.

That said, I agree we need something more consistent across actions, whether it's the promise returned by withDispatch (which has drawbacks: don't forget to cancel the promise when the component unmounts) or automatically identify and track the actions in core/data (like resolvers). I'm not certain yet what's the best approach.

I don't see it as crucial before 5.0 because there are workarounds but it's nice to have and if we get something in, that would be great.

@TimothyBJacobs

This comment has been minimized.

Contributor

TimothyBJacobs commented Oct 27, 2018

Gotcha. Thank you for the alternative! Agreed not necessary for 5.0, but would be nice. If something made it into the data module after API freeze, would it still be included in 5.0? Or is the deadline right around the corner.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 27, 2018

I think enhancements to the API are fine. Breaking changes not that welcome. But don't take my word for it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment