-
Notifications
You must be signed in to change notification settings - Fork 87
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
Add CRUD functionality to eventespresso/core
wp.data store.
#780
Conversation
2325054
to
ef2aab9
Compare
Things to consider: Example state for the const state = {
entities: {
event: {
10: { ...eventEntity },
},
datetime: {
'123124cuid': { ...datetimeEntity },
},
ticket: {
20: { ...ticketEntity }
}
},
relations: {
index: {
datetimes: {
'123124cuid': {
event: [ 10 ]
}
},
tickets: {
20: {
datetime: [ '123124cuid' ],
},
},
},
entityMap: {
event: {
10: {
datetimes: [ '123124cuid' ]
},
},
datetime: {
'123124cuid': {
tickets: [ 20 ],
}
},
}
},
dirty: {
relations: {
index: {
datetimes: {
40: {
event: { delete: [ 10 ] },
ticket: { delete: [ 20 ] },
},
'123124cuid': {
event: { add: [ 10 ] },
}
},
tickets: {
20: {
datetime: { add: [ '12314cuid' ] },
}
}
},
delete: {
event: {
10: {
datetimes: [ 40 ]
}
},
ticket: {
20: {
datetimes: [ 40 ]
}
}
},
add: {
event: {
10: {
datetimes: [ '123124cuid' ]
}
},
datetime: {
'123124cuid': {
tickets: [ 20 ],
}
}
}
},
trash: {
event: [ 20 ],
},
delete: {
datetime: [ 40 ],
}
}
}; Insert/UpdateThis will be handled by all dispatch actions.
Things needing worked out:
Deletes
Things needing worked out:
Relations
Things needing worked out:
|
Other tasks:Other tasks that might not be not directly related to the ticket but things discovered in the process of doing the work that I'm doing as a part of this pull
|
d582004
to
9be725c
Compare
05a459d
to
0da5c2d
Compare
28b1884
to
1a250e1
Compare
1a250e1
to
62b600b
Compare
f4e259c
to
4df4d8d
Compare
Checklist for
|
Checklist for
|
Checklist for
|
2dbbad1
to
af34ab1
Compare
e930f98
to
e4e5477
Compare
eventespresso/core
wp.data store.eventespresso/core
wp.data store.
- pop is more performant becuase the array does not need re-indexed. There’s no need to retain order here so `pop` is sufficient.
b80a76c
to
6d9a1b0
Compare
Ready for review again @tn3rb |
const listItems = attendees.map( | ||
( attendee ) => { | ||
return <AttendeeListItem | ||
key={ attendee.id || cuid() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't get why you are doing this and just not using the index generated by the call to map()
, ie:
const listItems = attendees.map(
( attendee, index ) => {
return <AttendeeListItem
key={ index }
The key can not be referenced in any way whatsoever so there is no reason for needing to be able to know what the key is.
Its only purpose is to guarantee uniqueness among the mapped items and the index does this sufficiently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may have missed my reply to your original comment which goes into a bit more explanation for why I took the path I did. Does that help with the reasons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nvm I see what you mean now.
const relationName = pluralModelName( relationData.relationName ); | ||
const relationIds = [ ...relatedEntityIds ]; | ||
while ( relationIds.length > 0 ) { | ||
const relationId = relationIds.shift(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed this one
@joshfeck (or whoever ends up testing) the main thing needing tested here is that there are no regressions with the Event Attendees block in the new WP editor. So:
The front-end rendering of blocks should remain unchanged as well. |
# Conflicts: # assets/dist/build-manifest.json # assets/dist/ee-components.622f86c8649619cbcbcb.dist.js # assets/dist/ee-components.7d6c99591998d2eae4c0.dist.js # assets/dist/ee-components.b72fa352434166ef52be.dist.js # assets/dist/ee-data-stores.67b185e1920ba24c7431.dist.js # assets/dist/ee-model.7f810d358d08f82e088b.dist.js # assets/dist/ee-model.93606ab921bb65bfe8e0.dist.js # assets/dist/ee-model.b08f7f3ee436d1dbb468.dist.js
Tested. Ready for merging. |
This was squash/merged manually as opposed to using the ui so closing. |
Problem this Pull Request solves
Now that we have the basic
wp.data
store(s) and model entity creation/management in place for javascript work in wp, its time to expand that to add some additionalCRUD
functionality (needed for REM work as well).In this pull:
eventespresso/core
store state shapeThe store state for
eventespresso/core
is given a shape described here. Essentially:The
eventespresso/core
state becomes the authority for all entities worked with in the request as well as the interface for crud actions.It should be noted that while the top level for the state are plain objects, the various trees in the state are immutable maps. The interface to the state (via actions and selectors) gives access to the data in the maps.
Selectors, Actions and Action Generators
All the selectors and actions for
eventespresso/core
are found described here. It should be noted that for any selectors having to calculate values or convert from immutable map, the selector is created using rememo. This adds a cache layer that:One thing I've taken care to do when it comes to action naming is that anything that results in data actually persisting data to the server is prefixed with
persist
:)Other stores
eventespresso/lists
andeventespresso/schema
also have changes using immutable and rememo. This required updates to our existing gutenberg block (and various components using those stores) so that they are correctly accounting for the changes.One important change related to that is that entities are now returned as a simple array of entities rather than a native javascript Map. This is slightly more performant as there really is no need to index by id when that's readily available from the data if/when needed. If we find we really NEED to have entities returned ordered and with keys being the ids, we can add a new selector to the store for that purpose.
Other changes
@wordpress/eslint-plugin
. This will help us with keeping up to date with the WP javascript standards with little effort on our end. We are still able to customize as needed.convertToMapFromObject
method as a part of the@eventespresso/helpers
module.getIdsFromBaseEntityArray
method as a part of the@eventespresso/helpers
module. This is a handy utility to extract an array of ids for all theBaseEntity
instances in a given array.normalizeEntityId
function to the@eventespresso/helpers
module. This function will leave a cuid alone and coerce anything else to an integer (usinglodash.toInteger
. This is used extensively in our stores.removeEmptyFromState
function to the@eventespresso/helpers
module that recursively clears empty things from a given object/Immutable.map. This is used extensively ineventespresso/core
reducers when removing things from the state.assertImmutableObjectHasPath
function to the@eventespresso/model
module. This does what it says, if the given path does not exist in the given immutable map/set/other then it throws an Exception.base_rest_route
value via theeejsdata
object. This in turn is accessed via theeejs.data
module. Thebase_rest_route
is essentially the root route path for all WP REST Requests. It is useful when the javascript client needs to strip it out from urls.@eventespresso/model
that strips the base_rest_route from a given url string:stripBaseRouteFromUrl
@eventespresso/model
:pluralModelName
andsingularModelName
that basically allow for normalizing to plural or singular for a given string. For instance if you dopluralModelName( 'person' )
you'll get backpeople
. This is used extensively throughout the stores code for normalizing model name references in the state. In the state, entity models names are always singular, whereas relation name references are always plural. While our api is somewhat inconsistent, we can get the correct slugs from the schema, in the state however I favoured consistency. This is especially useful because the the consuming code doesn't have to worry about what the correct slug is for a model, it can always use the singular model reference (or constants we can expose) and the store will automatically normalize to its correct reference. These functions are memoized, so you can call them repeatedly and it only does the conversion to singular/plural once for a given string. So keeps things optimized.getPrimaryKeyQueryString
is added to@eventespresso/model
. It basically returns the query string to use in a REST api request for the given model and primary key value(s). This is also memoized.InvalidModelEntity
exception to our@eventespresso/eejs
module@eventespresso/eejs
now exposes a middlewares property (eejs.middleWares
). This will contain any middlewares that are registered on demand on live page load. The first middlware in this iscapsMiddleware
which is invoked inline as middleware on thewp.apiFetch
module. This middleware ensures that the proper caps context is added to all outgoing EE REST_API requests.@wordpress/url
module. Take a gander at that module sometime, there's some useful utilities in there for working with urls. We can now use any of the functions exposed on that module by importing@wordpress/url
in our code.How has this been tested
EventAttendees
block. This is one thing that should receive additional testing before merging this branch in to master just to ensure I haven't broken anything that is user facing.For this third layer of testing I created checklists that I went through (see beginning here). For everything tested, I used redux dev tools extension to ensure the state had the expected changes. Anytime testing failed, I would write a jest test reproducing the fail and then fix it. That way I expanded on the jest tests to cover things I missed in the initial run through (and ensure any regressions get caught).
Remaining items for discussion/future work.
The following is a list of unresolved things thought of while working on this pull. I've decided that they can be worked on in future pulls and some may require additional discussion/planning:
wp.data
has a resolver cache reset api for resetting the resolved state for a selector with corresponding resolver. I could forsee this being something we may need in our stores as well (i.e. 'refreshing' the view on a page without a full page reload). I feel this is something that we can iterate on in future pulls as the need becomes more apparent.Checklist
esc_html__()
, see https://codex.wordpress.org/I18n_for_WordPress_Developers)