-
Notifications
You must be signed in to change notification settings - Fork 25
Conversation
* Make Patches and Queries have two generic parameters to express the original type as well as the resulting type. * Create a CompoundQuery class to allow for any arbitrary queries to be combined into a single query. * Remove the second generic type in BaseStore. For the most part, the type of the items in the store will not change between the source and the subStore, so they can use the same generic type. If they did change types, in one case the items could be mapped to a completely different type. In this case updates can no longer happen by delegating to the parent because there's no way to reverse engineer the relationship between the items. In the case where a selection or a similar operation has been performed, it would be useful, but the direction of the type relationship is contravariant. If the source has Items of type {a: string, b: string}, the child would have elements of either {a: string}, {a: string, b: string}, or {b: string}. In any other case we have the problem mentioned above. But in this case, the resulting item is a superclass of the source store. Which means the signature for this opertion would need to be something like ``` Store<A, B extends A> { source: Store<B> select<C extendedBy A>: Store<C, A>; } ``` But we can't express that.
* Add more options for serialization * Move queries to their own package * Add select query * Change pointer add method to push, and add pop * Change factory names to match compose pattern
* Change `type` in filter to `filterType`, query already has a queryType so this is hopefully more consistent/clearer. * Add store 'actions'. They're not necessarily proper actions but they express a similar pattern. They also, with the StoreActionManager, form the start of an attempt at handling conflicts and out of date data.
Store "Actions" are meant to wrap an update and allow for * Handling conflicts and other failures gracefully * Handle ordering of concurrent requests
Using an observer instead of a promise for a single action's result allows a lot of code passing promises through to be eliminated. It also simplified the interaction for an end user of the API, as retrying a failed request repeatedly no longer requires a potentially endless chain of promises.
Also includes refactoring
* Add another test for subcollections * Start adding more detailed tests for each method
…rsion of dependencies. The tsc compile issue is cased by some issue in the `next` tag of `dojo-core` package. Until a more stable version of `dojo-core` is released, we have to use previous working version for now.
Note: a few unit tests are still failing. Need to either update code to pass the unit tests, or remove the tests if we feel the tests are making wrong assumptions.
Action result 'errors' really represent data conflicts only, as other errors will still be handled through the error callback of the action. The relevant flag has been renamed to better reflect that. Action observers should only call `retryAll` or `filter` synchronously within the callback, and if they are not an appropriate error will be thrown. This is mainly so that the observer can be completed if the action is not going to be retried, by checking after its next callback whether `retryAll` or `filter` was called on the result. Actions now also throw appropriate errors when `retryAll` or `filter` is called more than once on the same result object, or on a successful result object. `Add` now calls `put` on retry, as retrying an add means that the item already exists but we want to update it anyway. Changed tests to reflect that: * Adding an existing item should be considered a data conflict, but the action should be able to be retried, and in that case it should update the existing item. * When subscribing to the store's observable, an initial update should be received that indicates the state of the store at the time of subscription. Fixes two other miscellaneous bugs: * `_add` in `MemoryStore` was adding all passed items to the store's data. Now it correctly only adds only the new items. * On retry, the action was being executed immediately instead of being queued.
The factory function `rangeFactory` is changed to `createRange` to be consistent with the rest of application such as filter#createFilter etc.
* Most modules has 100% coverage. * Overall coverage is over 80%, while a few corner cases are not covered yet. * Fix a few issues caught by tests. * Cleanup.
* Will update accordingly based on discussion in /dojo/compose/#81 * Attempt to fix build issue by adding `Rx.config.Promise`
Current coverage is 99.57% (diff: 99.57%)
@@ feature-new-stores #24 diff @@
====================================================
Files 1 17 +16
Lines 127 945 +818
Methods 1 5 +4
Messages 0 0
Branches 31 158 +127
====================================================
+ Hits 103 941 +838
+ Misses 23 2 -21
- Partials 1 2 +1
|
My thoughts on the newly added createOrderedOperationMixin.ts, along with the readme document. I see what problem
And IMO, we on the other hand, should not make the code above magically work by adding another mixin that does operation scheduling. If the user "want to be able to rely on the assumption that he will receive an update from one operation before the next operation occurs", he can simply put the next operation inside the
Or another way to do it, as in dstore 1, is to introduce a set of |
"homepage": "http://dojotoolkit.org", | ||
"bugs": { | ||
"url": "https://github.com/dojo/stores/issues" | ||
"name": "dojo-store-prototype", |
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.
this needs to be dojo-stores
"bugs": { | ||
"url": "https://github.com/dojo/stores/issues" | ||
"name": "dojo-store-prototype", | ||
"version": "0.0.1-alpha", |
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.
Can you update this to be 2.0.0-pre
?
}, | ||
"scripts": { | ||
"prepublish": "grunt peerDepInstall", | ||
"prepublish": "grunt peerDepInstall dist", |
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.
remove dist
"repository": { | ||
"type": "git", | ||
"url": "https://github.com/dojo/stores.git" | ||
"url": "https://github.com/dojo/core.git" |
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.
set back to stores
@@ -1,3 +1,22 @@ | |||
module.exports = function (grunt) { | |||
require('grunt-dojo2').initConfig(grunt); | |||
const gruntConfig = { | |||
dtsGenerator: { |
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.
don't need dtsGenerator anymore
main: 'dojo-stores/main' | ||
} | ||
}, | ||
watch: { |
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.
we should probably add a watch in grunt-dojo2
"dojo-core": ">=2.0.0-alpha.12", | ||
"dojo-has": ">=2.0.0-alpha.4", | ||
"dojo-shim": ">=2.0.0-alpha.4" | ||
"dojo-compose": "2.0.0-beta.11", |
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.
any reason why we are pinned to dojo-compose beta.11?
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 think so. Please see dojo/compose#81 for details.
export default JsonPointer; | ||
|
||
export function navigate(path: JsonPointer, target: any) { | ||
return path.segments().reduce(function(prev: any, next: string) { |
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.
why any
? also anonymous functions should use arrows.
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.
Point 5 under the general coding style guidelines indicates that arrow functions should only be used to preserve execution context. I take it that that is not correct?
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.
argh has @kitsonk not updated that yet...
op: string; | ||
path: JsonPointer; | ||
toString(): string; | ||
apply(target: any): any; |
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.
any
again?
export default Operation; | ||
|
||
export interface Add extends Operation { | ||
value: any; |
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.
Presume we could deal with the type?
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'm not sure we can in a meaningful way. Patch
allows for types to be specified, and it exports a diff
method that will infer those types from its arguments. That's the primary way that the JSONPatch stuff will be used.
But Add
is adding a property at an arbitrary path in an object. If I wanted to create the
operation:
{ "op": "add", "path": "/a/b/c", "value": [ "foo", "bar" ] }
I'm not sure how feasible it would be, or how much sense it would make, to try to enforce any kind of typing there.
* Wipe out files for merging new stores. * Starting work on new store * Start conversion to use observables, and incorporate dojo-2-package template * Compiles with Memory and Request Stores commented out - WIP * Fix dependency issues with new version of grunt-dojo2 * Update some dependencies and fix folder case * Fix capitalization, make interfaces default exports * Refactoring * Make Patches and Queries have two generic parameters to express the original type as well as the resulting type. * Create a CompoundQuery class to allow for any arbitrary queries to be combined into a single query. * Remove the second generic type in BaseStore. For the most part, the type of the items in the store will not change between the source and the subStore, so they can use the same generic type. If they did change types, in one case the items could be mapped to a completely different type. In this case updates can no longer happen by delegating to the parent because there's no way to reverse engineer the relationship between the items. In the case where a selection or a similar operation has been performed, it would be useful, but the direction of the type relationship is contravariant. If the source has Items of type {a: string, b: string}, the child would have elements of either {a: string}, {a: string, b: string}, or {b: string}. In any other case we have the problem mentioned above. But in this case, the resulting item is a superclass of the source store. Which means the signature for this opertion would need to be something like ``` Store<A, B extends A> { source: Store<B> select<C extendedBy A>: Store<C, A>; } ``` But we can't express that. * Refactoring * Add more options for serialization * Move queries to their own package * Add select query * Change pointer add method to push, and add pop * Change factory names to match compose pattern * Fix testing configuration and dist build * Add a few simple unit tests for sort. * Add store 'actions', minor refactoring * Change `type` in filter to `filterType`, query already has a queryType so this is hopefully more consistent/clearer. * Add store 'actions'. They're not necessarily proper actions but they express a similar pattern. They also, with the StoreActionManager, form the start of an attempt at handling conflicts and out of date data. * Adding in "actions" Store "Actions" are meant to wrap an update and allow for * Handling conflicts and other failures gracefully * Handle ordering of concurrent requests * Use an observer instead of a promise for a single action's results Using an observer instead of a promise for a single action's result allows a lot of code passing promises through to be eliminated. It also simplified the interaction for an end user of the API, as retrying a failed request repeatedly no longer requires a potentially endless chain of promises. * Implement memory store and add some unit tests for it Also includes refactoring * Adding tests - in a broken state * More tests * Minor changes to make code compile. * Fix dojo dependency versions * Add more unit tests * Add another test for subcollections * Start adding more detailed tests for each method * bump peer deps to 'next' tag and config for UMD typings * change deps to peer deps in line with dojo2 convention, add grunt task for peer deps * update .gitignore for node_modules and .tsconfig*.json files * implement 'noImplicitThis' * Fix the build(compile) issue by switching back to previous working version of dependencies. The tsc compile issue is cased by some issue in the `next` tag of `dojo-core` package. Until a more stable version of `dojo-core` is released, we have to use previous working version for now. * Add more unit tests for MemoryStore. Fix minor code issue. Note: a few unit tests are still failing. Need to either update code to pass the unit tests, or remove the tests if we feel the tests are making wrong assumptions. * Minor API change, fixes to Actions, add method, and tests Action result 'errors' really represent data conflicts only, as other errors will still be handled through the error callback of the action. The relevant flag has been renamed to better reflect that. Action observers should only call `retryAll` or `filter` synchronously within the callback, and if they are not an appropriate error will be thrown. This is mainly so that the observer can be completed if the action is not going to be retried, by checking after its next callback whether `retryAll` or `filter` was called on the result. Actions now also throw appropriate errors when `retryAll` or `filter` is called more than once on the same result object, or on a successful result object. `Add` now calls `put` on retry, as retrying an add means that the item already exists but we want to update it anyway. Changed tests to reflect that: * Adding an existing item should be considered a data conflict, but the action should be able to be retried, and in that case it should update the existing item. * When subscribing to the store's observable, an initial update should be received that indicates the state of the store at the time of subscription. Fixes two other miscellaneous bugs: * `_add` in `MemoryStore` was adding all passed items to the store's data. Now it correctly only adds only the new items. * On retry, the action was being executed immediately instead of being queued. * Add grunt watch task registered as the default task. * Add unit tests around `store#fetch`. The factory function `rangeFactory` is changed to `createRange` to be consistent with the rest of application such as filter#createFilter etc. * fix TS2 version and bump dojo deps (dojo#7) * Minor refactor and more tests * Minor changes to update interface so that all updates can optionally have an item or index property. This avoids a lot of casting to different update types. * Updates to track and release API so that they return a promise to the store. * Don't send out an initial update of current items when observing the store. If using fetch, it's not performant, and if not using fetch, the results can be surprising because the local data may not be what is expected at the time of observation. * Getting rid of fat arrow functions to match dojo style guide * Making the order of actions more controllable via custom store managers by adding a retry function. The default manager currently just executes the next available action asynchronously as soon as the current action executes once. Any retries on that action are treated as new actions and just queued normally. With the separate retry method an action manager could be implemented that waits until the current action is completed, including waiting for any retries. * Refactoring API and breaking out into separate compose mixins * Separate query module into a mixin. Use instance constructor as subcollection factory Make observableStoreMixin subcollection aware * Add more unit tests for query mixin * Fix query mixin so it doesn't query recursively * Adding track and release mixin - No tests yet * Add unit tests from previous code. Add unit tests for track mixin. (doesn't compile yet) Add unit tests for Patch/JsonPointer. * achieved 100% coverage. * a few issues were caught and fixed. * Interface cleanup and test case for trackable mixin Fix ordering for operations and tracking. Doesn't resolve the case where fetching occurs around the operation. * Add more test cases for and fix issues with operation ordering * Split else onto a separate line * Update readme * Fix internal link in readme * Really fix internal link in readme * Add unit tests. * Most modules has 100% coverage. * Overall coverage is over 80%, while a few corner cases are not covered yet. * Fix a few issues caught by tests. * Cleanup. * Fix build issues. * Comment out failing unit tests. See dojo/compose/dojo#81 for details. * Add missing all.ts to fix build issue. * Switch back to previous version of compose, uncomment unit tests. * Will update accordingly based on discussion in /dojo/compose/dojo#81 * Attempt to fix build issue by adding `Rx.config.Promise` * Fix build issues on IE 10/11 caused by unavailable functionality. * Update IOS version due to issues with saucelabs environments. * Update IOS version to 9.3 due to issues with saucelabs environments. * Trigger Build. (Previous build dojo#32 seems to hang for no reason.) * Add more tests to cover more areas of logic. * Feedback changes: package name and version etc. * Move file into mixin dir. * Add createTransactionMixin. * Remove dtsGenerator grunt config * Add tests for createTransactionMixin. Fix issues caught by tests.
Type: feature
Description:
This is all the work that has done based on previous reviews. A few highlights:
Most work is done by @maier49.
All the commits in this PR are originally from maier49/store, due to the two repositories are completely different - they don't have related history, to preserve history of all the work we have done, I did a bit research and came up with this PR. Please let me know if there's any issues.
Any feedback/comments on anything we have done is highly appreciated.
Please review this checklist before submitting your PR: