Transform query #67
Transform query #67
Conversation
2725c81 to
400cabe
Compare
Current coverage is 100% (diff: 100%)@@ master #67 diff @@
======================================
Files 17 15 -2
Lines 993 1096 +103
Methods 6 6
Messages 0 0
Branches 177 185 +8
======================================
+ Hits 974 1096 +122
+ Misses 1 0 -1
+ Partials 18 0 -18
|
babac90 to
9ea5c0d
Compare
65242e8 to
5ac0f02
Compare
| @@ -33,36 +49,56 @@ const createCompoundQuery: QueryFactory = compose<CompoundQuery<{}>, QueryOption | |||
| }, data)); | |||
| }, | |||
|
|
|||
| withQuery(this: Query<{}>, query: Query<{}>): CompoundQuery<{}> { | |||
| withQuery(this: CompoundQuery<{}>, query: Query<{}>): CompoundQuery<{}> { | |||
There was a problem hiding this comment.
Sorry if this is completely wrong but could withQuery<T>(this: CompoundQuery<T>, query: Query<T>): CompoundQuery<T> { work? I see we use {} all the time in stores to satisfy generics and wonder if there is any point in having the generics if we always just internally specify {} or any as the generic.
There was a problem hiding this comment.
The problem is that there's no generic around to use. I agree that it's not ideal, but this is the pattern I gleaned from widgets wherever generics are being used in the mixin or factory.
For some mixins I've been using the pattern of having a function return the mixin which allows the generics to be specified, but for a factory I think having a function to call to return a factory would be weird. I don't think the generics are completely pointless though. It will still make sure that you're passing the right type of Query to your compoundQuery.withQuery, and providing the right type of Query to a Store.
One possible approach to keep generics in a bit more of the code would be to have something like this.
function createCompoundQueryType<T>(): CompoundQuery<T> {
withQuery(this: CompoundQuery<T>, query: Query<T>): CompoundQuery<T> {
}
}
const createCompoundQuery: QueryFactory = compose<CompoundQuery<{}>, QueryOptions<{}>>(
createCompoundQueryType<{}>(),
initialize() {
}
);That would still need to be passed {} or any when it's provided to the factory, but within the function it would have to respect whatever the type boundaries of the generic were.
There was a problem hiding this comment.
Yeah I think we need to look at generics usage in general with compose because at the moment I don't think this offers very much :(
There was a problem hiding this comment.
In this case I'm not sure we're really losing much utility from the generics. I think the main benefit of the generics in queries in general is making sure the right type of query is being passed to the store, or combined in CompoundQuery, and that in the case of custom filters the object being examined has the appropriate type.
All of those goals should still be met by this and the actual data isn't being manipulated much in most of the these factories anyways so the type of the data isn't really a big concern.
But I agree it's a concern in general.
Splits out querying to a seprate interface that also provides transform functionality Transforming can reuslt in two separate interfaces: If a transformation is applied to data, the existing id property/function cannot be trusted. Adding an optional parameter to the transformation that allows an id property or function to be specified allows identification and tracking(which relies on IDs for determining which item is in what position) to work, but also requires splitting off the identify and track methods to a separate interface for when transform is used without passing an id property or function. This also removes the ordering mixin, as it was really more of a kluge than initially realized. The memory store now makes sure that operations resolve in the order they were executed, without controlling the order of execution. The ordering of promise resolution in the observable mixin has also been updated to maintain this. If for some reason a remote storage does actually return operations in a different order than they were executed then there could still potentially be issues. But other than marking the execution order in the return values from the server there might not be a good way to resolve that. Either way it should be a rare issue if it occurs at all. This still needs tests for transformations.
The observable mixin always includes the before/after properties now and sends updates after fetching. If fetchAroundUpdates if false and a manual fetch hasn't been called the store will update its collection in place and use that for before/after properties on updates. The `adds`, `updates`, and `deletes` properties are not affected by this change and are only used to report specific operations(i.e. put for update, add for add, delete for delete). The QueryTransformResult now always sends a "Tracked" update, unless it is not "mapped". A non "mapped" result is one where a transform was provided without indicating how to restore the ID. In this case the objects cannot be identified and indices can't be stored in a hash table so sending tracked updates would be prohibitively expensive. The tracked update will apply the local queries to the beforeAll and/or afterAll properties if available, and will fetch for data only if these are not sufficient and tracking is explicitly enabled. The before/after properties will be sufficient any time that the source store is using fetchAroundUpdates = true, since that means the full data will be available to the filtered view. Both the store observable mixin and the query/transform observables now publish an update to the subscriber when they first subscribe. adds/updates/deletes properties are not used for this. For the observable mixin all the local data will be in `afterAll` and for the tracked/queries view the local data will be in `afterAll` and position information on individual items will still be accessible through the `addedToTracked`, `removedFromTracked`, and `movedInTracked` properties.
187f009 to
8264933
Compare
Type: feature
Description:
Provides a transform and query API. This represents a fairly large refactor because the Transform/Query API is build on top of the observable API, requiring some changes there to make it properly function, and it also eliminates the
createSubcollectionStore,createTrackableMixin, andcreateQueryMixinmodules as they were together accomplishing more or less the querying half of transform and query.The change to
Query's signature, could be merged separately, reducing the surface area of this review. Those changes already have a PR open #43 . Other than that it might be difficult to separate this into separate PRs.Related Issue: #58 #53 #39 #20 #19
Please review this checklist before submitting your PR: