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

Update get and identify APIs to return the item/id in one item case. #61

Merged
merged 2 commits into from Dec 9, 2016
Merged

Update get and identify APIs to return the item/id in one item case. #61

merged 2 commits into from Dec 9, 2016

Conversation

lzhoucs
Copy link
Contributor

@lzhoucs lzhoucs commented Dec 1, 2016

Type: feature

Description:
Update get and identify APIs to return the item/id instead of the one item.id array in one item case.

Related Issue: #60

  • 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

@@ -83,12 +86,16 @@ function isPatch(patchObj: any): patchObj is {id: string; patch: Patch<any, any>
}

const createStore: StoreFactory = compose<Store<{}, {}, any>, StoreOptions<{}, {}>>({
get(this: Store<{}, {}, any>, ids: string[] | string): Promise<{}[]> {
get(this: Store<{}, {}, any>, ids: string[] | string): Promise<any> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we type the return here to Promise<{} | {}[]>? That way it provides at least some guarantee about the return type in the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I have made the change.

@@ -222,8 +229,14 @@ const createStore: StoreFactory = compose<Store<{}, {}, any>, StoreOptions<{}, {
});
},

identify(this: Store<{}, {}, any>, items: {}[] | {}) {
return instanceStateMap.get(this).storage.identify(Array.isArray(items) ? items : [ items ]);
identify(this: Store<{}, {}, any>, items: {}[] | {}): any {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here really, could this return string | string[]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although get can be done this way, identify can't as I tried. Please let me know if you have ways to get around this and I am happy to make it type safer.

@lzhoucs
Copy link
Contributor Author

lzhoucs commented Dec 2, 2016

Just a note, the CI has been hanging on iphone 9.3 which is not caused by changes in this PR:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

I've seem this happen before. It seems it only fails occasionally.

@dylans dylans added this to the 2016.12 milestone Dec 5, 2016
@maier49 maier49 merged commit be83f1b into dojo:master Dec 9, 2016
@lzhoucs lzhoucs deleted the minor-apis-change branch December 9, 2016 17:03
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.

None yet

3 participants