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

Create wp.data store for retrieving/tracking events #332

Merged
merged 17 commits into from May 1, 2018

Conversation

Projects
None yet
2 participants
@nerrad
Copy link
Member

nerrad commented Apr 24, 2018

Note: for reference, original ticket is https://events.codebasehq.com/projects/event-espresso/tickets/11472

While working on creating an EventSelector component, I realized that although the withApiData HOC WP provides is useful, if that selector is used on multiple places (blocks etc), then every time the selector is loaded there's an api request to get the list of events. That could be problematic (especially when something like this lands on EventSmart). The wp.data feature in Gutenberg (which will eventually be a standalone package) will be useful for this because I can just register a custom store for this type of query and have it memoize (using resolvers I think) by some sort of hash based on the query. That way the same query for the events in the selector will just return the existing events object in the store.

So as I think about this, there are some things I want to do:

  1. Keep a top level store that keeps tracks of entities already retrieved and being worked with. For example ee-core store might have a top level events entities state that serves as the "authoritative" state for those event entities.
  2. Need to have a way to record whether the state for an entity is dirty (i.e. ee-core.events.1.object would have the event entity itself and ee-core-events.1.dirty would be a boolean indicating whether its "dirty" (not in sync with the db = true) or not.
  3. By default, any reducers in any store dealing with ee entities would need to go through some sort of middleware or otherwise so that depending on the purpose of that reducer (for example a reducer that is not intended to mutate the state of entities in the ee-core store), it will optionally mutate/not mutate entities if they exist (but still add new ones if they don't exist). So for example, for this ticket I'm creating an ee-list store that is mostly for just retrieving lists of model entities from our api and populating the ee-core store from what is retrieved. However ee-list resolvers and reducers will by default not mutate the entities in the ee-core store because they are just intended for querying for things like select dropdowns, checkboxes etc. Things where if elsewhere the entity details are changed (i.e. Event Name) it may be desired to update that automatically within any objects referencing that store.
  4. We may still want a way to optionally flush the dirty state of entities within the ee-core store (undo changes, or at least make it allowable to "restore" the state from some other source (db, local storage etc).

Also noting. For the resolvers, I'm using a variation of the async generator pattern that WP core is using for their wp-core data resolvers. For reference here's some documentation I've been reading following for things related to them:

@nerrad

This comment has been minimized.

Copy link
Member Author

nerrad commented Apr 24, 2018

Another potential wrinkle I may have to work through is doing some sort of paging where there are large data-sets - it's likely we'll want to use some sort of select2 type interface. Worth noting is the conversation surrounding this here within the Gutenberg project.

@nerrad nerrad changed the title Create wp.data store for retrieving/tracking events. Create wp.data store for retrieving/tracking events (wip) Apr 24, 2018

@nerrad nerrad closed this Apr 24, 2018

@nerrad nerrad force-pushed the Gutenberg/create-wp.data-store-for-retrieving-events branch from 8372075 to f1f1710 Apr 24, 2018

@nerrad nerrad reopened this Apr 24, 2018

},
'@wordpress/data': {
this: [ 'wp', 'data' ],
},

This comment has been minimized.

@nerrad

nerrad Apr 24, 2018

Author Member

doing this allows us to import as aliased package names in our source files and then if/when WrodPress releases these things as actual packages, we just have to update the webpack config and all our source files can keep the existing imports.

@nerrad nerrad force-pushed the Gutenberg/create-wp.data-store-for-retrieving-events branch 5 times, most recently from ebc67d6 to 4dec8a5 Apr 25, 2018

nerrad added some commits Apr 24, 2018

First pass of js side of things for the list wp.data implementation
`src/data/model/endpoints` expose:

- `getEndpoint` (default) for getting the endpoint for a model name (via the server side generated endpoints array exposed on the `eejs.data` global.
- `endpoints` : the full endpoints array.
- `applyQueryString`: which receives a model name and a query string and applies the query string to the endpoint corresponding to that model name and returns the final assembled endpoint.

`src/data/ee-lists/…`

- selectors for entities attached to a specified model and cached by query string
- reducers for updating the store state for lists cached by query string
- resolvers for the initial api query to retrieve entities matching the query string
- actions describing the various actions that can be dispatched to update the store state.

nerrad added some commits Apr 26, 2018

Install gutenberg as a devDependeny
The goal here is to be able to do jest tests for things requiring gutenberg dependencies.
Add initial test for EventSelect component
This was a fun one to do because I basically had to figure out how I could load in gutenberg components that were composed in the component I wanted to test!
Remove unnecessary mock
This is taken care of globally so don’t need it in this file specifically.

@nerrad nerrad force-pushed the Gutenberg/create-wp.data-store-for-retrieving-events branch from 793503c to 29e303f Apr 30, 2018

@nerrad
Copy link
Member Author

nerrad left a comment

The work in this ticket is preliminary foundation work that will be used across various shortcode to block conversions we do. Initially, it implemented in the EventSelector control and I anticipate it being used in other components at some point.

So assuming js tests pass and it passes a code review I'd like to get this merged into the Gutenberg/master branch as it will not break anything currently in it. Then as we implement this in other work we can continue to make any necessary tweaks and/or improvements as necessary.

*/
EventSelect.propTypes = {
events: PropTypes.shape( {
events: PropTypes.arrayOf( PropTypes.shape( {

This comment has been minimized.

@nerrad

nerrad Apr 30, 2018

Author Member

The original implementation of this was incorrect, so this fixes that (covered by tests as well).

This comment has been minimized.

@tn3rb

tn3rb May 1, 2018

Member

Since it's very likely that we will repeatedly use this kind of object for various selectors, what about using a more formally declared class ?
This could even be an abstract parent with concrete children for defining the specific types. ie:

  • abstract SelectorKeyValuePair class
  • EventSelectorKeyValuePair extends SelectorKeyValuePair
  • DatetimeSelectorKeyValuePair extends SelectorKeyValuePair
  • AttendeeSelectorKeyValuePair extends SelectorKeyValuePair
  • etc

otherwise we will have to import the prop-types package and write this same code over and over in every component that uses selectors.

This comment has been minimized.

@nerrad

nerrad May 1, 2018

Author Member

I have no idea how this class will take shape yet. I'm also not sure we'd need a class yet. I'd like to avoid doing anything until we see patterns emerge. I think it's too early to yet.

} ),
onEventSelect: PropTypes.func.required,
} ) ),
onEventSelect: PropTypes.func,

This comment has been minimized.

@nerrad

nerrad Apr 30, 2018

Author Member

made this not required because there's validation upstream within the SelectControl component.

@@ -101,6 +103,9 @@ EventSelect.defaultProps = {
order: 'desc',
showExpired: false,
},
selectLabel: __( 'Select Event', 'event_espresso' ),
isLoading: true,
events: [],

This comment has been minimized.

@nerrad

nerrad Apr 30, 2018

Author Member

I moved the defaults here instead of in the constructor for the component (since I was already defining defaults here).

events: `/ee/v4.8.36/events?${ queryString }`,
isLoading: false,
events: getEvents( queryString ),
isLoading: isRequestingEvents( queryString ),

This comment has been minimized.

@nerrad

nerrad Apr 30, 2018

Author Member

This is implementing the new HOC that was done in this branch.

@@ -0,0 +1,40 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`EventSelect with 2 events should render and match snapshot 1`] = `

This comment has been minimized.

@nerrad

nerrad Apr 30, 2018

Author Member

This is a snapshot created by Jest and the new tests I added. This should never get modified directly.

import * as actions from './actions';
import * as resolvers from './resolvers';

const store = registerStore( 'eventespresso/lists', {

This comment has been minimized.

@nerrad

nerrad Apr 30, 2018

Author Member

The intended purpose for the eventespresso/lists store is for any components that need to retrieve a list of EE entities for given queries for use in elements of that component. The intent being that the retrieved entities are cached by that query so if there are multiple instances of that query requested in the same view, we avoid extra hits to the REST API endpoint for that query.

$this->jsdata['paths'] = array(
'rest_route' => rest_url('ee/v4.8.36/'),
'collection_endpoints' => EED_Core_Rest_Api::getCollectionRoutesIndexedByModelName()
);

This comment has been minimized.

@nerrad

nerrad Apr 30, 2018

Author Member

This is provided so there's an easy way for any of our React work to receive a list of collection endpoints for setting up EE REST API queries.

This comment has been minimized.

@tn3rb

tn3rb May 1, 2018

Member

if EED_Core_Rest_Api::getCollectionRoutesIndexedByModelName() is an expensive operation,
then maybe we should inject the Request into this class and only add this if Request->isApi() is true ?

This comment has been minimized.

@nerrad

nerrad May 1, 2018

Author Member

Hmm I don't think it's a super expensive request. The unfortunate thing is I don't think your suggestion would work because this is actually getting generated on a normal browser request not an api request. It's to provide info to the js for making api requests.

This comment has been minimized.

@tn3rb

tn3rb May 1, 2018

Member

ahhh kk then nvmd

"pegjs": "^0.10.0",
"pegjs-loader": "^0.5.4",

This comment has been minimized.

@nerrad

nerrad Apr 30, 2018

Author Member

This is all related to tests implemented for gutenberg type testing. I anticipate that we'll only be doing this type of testing mostly within the gutenberg branch and thus can just get merged with the shortcode to block conversion but if it turns out this is needed in any future work on master I'll have to cherrypick changes to master and work out the conflicts when merging upstream.

],
"preset": "@wordpress/jest-preset-default",
"testPathIgnorePatterns": [
"/node_modules/",
"/test/e2e"
],
"transformIgnorePatterns": [
"node_modules/(?!(gutenberg)/)"
]
}

This comment has been minimized.

@nerrad

nerrad Apr 30, 2018

Author Member

I wrote a blog post about all the changes in this file here: http://unfoldingneurons.com/2018/testing-javascript-for-your-gutenberg-integrated-plugin

@nerrad nerrad requested a review from tn3rb Apr 30, 2018

@nerrad nerrad assigned tn3rb and unassigned nerrad Apr 30, 2018

@tn3rb tn3rb added the status:review label Apr 30, 2018

*/
EventSelect.propTypes = {
events: PropTypes.shape( {
events: PropTypes.arrayOf( PropTypes.shape( {

This comment has been minimized.

@tn3rb

tn3rb May 1, 2018

Member

Since it's very likely that we will repeatedly use this kind of object for various selectors, what about using a more formally declared class ?
This could even be an abstract parent with concrete children for defining the specific types. ie:

  • abstract SelectorKeyValuePair class
  • EventSelectorKeyValuePair extends SelectorKeyValuePair
  • DatetimeSelectorKeyValuePair extends SelectorKeyValuePair
  • AttendeeSelectorKeyValuePair extends SelectorKeyValuePair
  • etc

otherwise we will have to import the prop-types package and write this same code over and over in every component that uses selectors.

return state;
}

export default listItems;

This comment has been minimized.

@tn3rb

tn3rb May 1, 2018

Member

hard for me to review this because I'm not grokking what is happening here, or what reducers are even supposed to do...

This comment has been minimized.

@nerrad

nerrad May 1, 2018

Author Member

Ya it takes some reading :) What I did was read up on redux and flux patterns where many of the concepts are outlined. Then I took a hard look at what the Guteberg project is doing (see the README.md and code here: https://github.com/WordPress/gutenberg/tree/master/data)

*/
export function getEvents( state, queryString ) {
return getItems( state, 'event', queryString );
}

This comment has been minimized.

@tn3rb

tn3rb May 1, 2018

Member

should this last method go in the same file with the generic getItems() method?
and are we going to populate this same file with similar methods for other entity types? ie: getDatetimes()

This comment has been minimized.

@nerrad

nerrad May 1, 2018

Author Member

should this last method go in the same file with the generic getItems() method?

Well it IS in the same file as the getItems async genrator and that's what it is calling :). No the resolvers have to have the same function names as the selectors and they get "automagically" assembled together as a part of the store object when registering them with wp.data.registerStore. In other words, a resolver that matches a selector will query the api ONCE to populate the store on the first call to that selector. Then further calls to the selectors will just retrieve data from the store.

and are we going to populate this same file with similar methods for other entity types?

possibly. I haven't decided yet. I want to start using this in actual implementations before I decide on an appropriate pattern. The tricky part here is that as I pointed out above there must be a matching resolver for any selector that needs populated via the REST api on the initial query.

Another spoke to this (which isn't found in this branch yet) is adding actions that are dispatched to the store for updating data. I don't want to start adding those actions until we start seeing how this will work in full implementations.

So I've focused on having the generic functions and a single concrete (event) to begin using and then we'll add more as we need them. Once we start seeing patterns emerge we can work on implementing more DRY architecture.

This comment has been minimized.

@tn3rb

tn3rb May 1, 2018

Member

should this last method go in the same file with the generic getItems() method?

Well it IS in the same file as the getItems async genrator and that's what it is calling :).

sigh... I realize that there are a lot of aspects to this new JS code that I don't understand and I am more than willing to admit when there is something I'm not grokking... but I am however fully capable of recognizing when two functions are in the same file, especially because I almost always checkout the branch and look at the actual code when dealing with a complicated diff or code that is more challenging. In this case it's also obvious that those two functions are in the same file because if they were not, then one of them would have to be called via an imported object (ie: object.getItems()) right?

I think it would benefit you greatly in the future to give people the benefit of the doubt, and take a few seconds every now and then to consider if maybe the person you're about to respond to actually does understand the situation, and if maybe there is some other way that you could interpret what they have written in a way that doesn't presume them to be completely void of intelligence.

I fully understood that those two functions were in the same file because I was looking at the actual code in my IDE. So what I was actually asking was SHOULD that last method go in the same file with the generic getItems() method... as in... does it belong there or should it go in its own file? In other words, I wanted to know if the plan was to have ALL of the resolvers in one file, or to separate specific entity types into their own files, ie: a generic parent resolvers.js plus event-resolvers.js, datetime-resolvers.js, attendee-resolvers.js, registration-resolvers.js, transaction-resolvers.js, etc, etc, etc.

The last sentence you wrote above is sufficient in answering my question and there is no need to respond to this.

This comment has been minimized.

@nerrad

nerrad May 1, 2018

Author Member

I think it would benefit you greatly in the future to give people the benefit of the doubt, and take a few seconds every now and then to consider if maybe the person you're about to respond to actually does understand the situation, and if maybe there is some other way that you could interpret what they have written in a way that doesn't presume them to be completely void of intelligence.

And maybe it would benefit you greatly in the future if you don't immediately jump to the conclusion that someone is criticizing your intelligence. My statement was intended clarify what thought you meant by "same file" since in one paragraph it seemed to refer to different things. I'm left asking "What "same file are you referring to'?", "Is he asking if this should be in the same file as selectors.js (which also has a getEvents method already)?". In the light of vagueness, I made a statement that was a segue into how the methods functioned which hopefully would have clarified why they were named as is. I honestly did not know what you were referring to or the context of your question so thought my reply might help expedite the conversation and clarify what's happening in these files. It definitely was not intended in the way you seem to have taken it by your reply.

I hope there could be a reconsideration of the default way my comments seem to be perceived. My goal is to help advance the project not put others down.

*/
export function isRequestingEvents( state, queryString ) {
return isRequestingItems( state, 'event', queryString );
}

This comment has been minimized.

@tn3rb

tn3rb May 1, 2018

Member

similar to my last question above, are we going to populate this same file with similar methods like getDatetimes() and isRequestingDatetimes() ?
would those be better grouped by entity type?
ie: have one file for all event related selectors, resolvers, etc and another for datetimes, etc

This comment has been minimized.

@nerrad

nerrad May 1, 2018

Author Member

Much like the other comments I've made so far, ya we may end up with that, but I think its too early to scope that out yet. I'd like us to watch for patterns to emerge first.

},
} );
} );
} );

This comment has been minimized.

@tn3rb

tn3rb May 1, 2018

Member

have yet to read anything about Jest, so am not reviewing this. (not saying it can't be merged, just saying that I have no comment on this code)

$this->jsdata['paths'] = array(
'rest_route' => rest_url('ee/v4.8.36/'),
'collection_endpoints' => EED_Core_Rest_Api::getCollectionRoutesIndexedByModelName()
);

This comment has been minimized.

@tn3rb

tn3rb May 1, 2018

Member

if EED_Core_Rest_Api::getCollectionRoutesIndexedByModelName() is an expensive operation,
then maybe we should inject the Request into this class and only add this if Request->isApi() is true ?

@nerrad nerrad requested a review from tn3rb May 1, 2018

@nerrad nerrad changed the title Create wp.data store for retrieving/tracking events (wip) Create wp.data store for retrieving/tracking events May 1, 2018

@nerrad nerrad removed the request for review from tn3rb May 1, 2018

@tn3rb tn3rb self-requested a review May 1, 2018

@tn3rb

tn3rb approved these changes May 1, 2018

@tn3rb tn3rb assigned nerrad and unassigned tn3rb May 1, 2018

@nerrad nerrad merged commit 45f2c5e into Gutenberg/master May 1, 2018

3 checks passed

WIP ready for review
Details
codecov/project 38.83% (+34.66%) compared to 632ce5b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nerrad nerrad deleted the Gutenberg/create-wp.data-store-for-retrieving-events branch May 1, 2018

@nerrad nerrad removed the status:review label May 1, 2018

@nerrad nerrad modified the milestone: 4.9.62 May 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.