Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Review: changes for the new stores #24

Merged
merged 58 commits into from
Oct 18, 2016
Merged

Review: changes for the new stores #24

merged 58 commits into from
Oct 18, 2016

Conversation

lzhoucs
Copy link
Contributor

@lzhoucs lzhoucs commented Oct 12, 2016

Type: feature

Description:

This is all the work that has done based on previous reviews. A few highlights:

  • Converted TS classes into compose factories
  • Separated functionality into mixins.
  • Test coverage is over 80%.

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:

  • There is a related issue
  • All contributors have signed a CLA
  • All code matches the style guide
  • The code passes the CI tests
  • Unit or Functional tests are included in the PR
  • The PR increases or maintains the overall unit test coverage percentage
  • The code is ready to be merged

lzhoucs and others added 30 commits October 12, 2016 03:03
* 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.
* 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.
@codecov-io
Copy link

codecov-io commented Oct 13, 2016

Current coverage is 99.57% (diff: 99.57%)

Merging #24 into feature-new-stores will increase coverage by 18.47%

@@           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   

Powered by Codecov. Last update f604208...325d670

@lzhoucs
Copy link
Contributor Author

lzhoucs commented Oct 14, 2016

My thoughts on the newly added createOrderedOperationMixin.ts, along with the readme document.

I see what problem createOrderedOperationsMixin is trying to solve. but my understanding is that it is not a problem that we should solve. No matter how sync the operation really is based on its underlying storage, the store is returning a promise, as it should based on its API. So the user also should not assume the then callback for the 1st put call can be called before the 2nd put call just because the memory storage operates synchronously. And the user can't just put code like below and expect it to work, because that is not the nature of how async promise work.

store.put(newItem).then(function() {
   // logic need to happen before the 2nd put call below
});

store.put(anotherItem);

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 then callback, or chain it with another then callback:

// 1st way
store.put(newItem).then(function() {
   // logic need to happen before the 2nd put call below
    store.put(anotherItem);
});
// 2nd way
store.put(newItem)
.then(function() {
   // logic need to happen before the 2nd put call below
})
.then(function() {
  // store.put(anotherItem);
})

Or another way to do it, as in dstore 1, is to introduce a set of sync version of APIs, putSync etc.

"homepage": "http://dojotoolkit.org",
"bugs": {
"url": "https://github.com/dojo/stores/issues"
"name": "dojo-store-prototype",
Copy link
Member

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",
Copy link
Member

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",
Copy link
Member

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"
Copy link
Member

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: {
Copy link
Member

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: {
Copy link
Member

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",
Copy link
Member

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?

Copy link
Contributor Author

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) {
Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Member

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;
Copy link
Member

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;
Copy link
Member

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?

Copy link
Contributor

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.

@agubler agubler merged commit cdc2fe3 into dojo:feature-new-stores Oct 18, 2016
agubler pushed a commit to agubler/stores that referenced this pull request Oct 18, 2016
* 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.
@dylans dylans added this to the 2016.10 milestone Oct 27, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants