This repository has been archived by the owner. It is now read-only.

Type store contents #143

Merged
merged 5 commits into from Nov 30, 2017

Conversation

Projects
None yet
4 participants
@maier49
Contributor

maier49 commented Nov 21, 2017

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:
A first attempt at providing better type inference and type checking for stores. If a command factory is used, the return types of get, and the paths provided to path will all be inferred and verified against the state type provided to the command factory.

Resolves #142

@maier49 maier49 requested review from agubler and matt-gadd Nov 21, 2017

/**
* The private state object
*/
private _state: object = {};
private _state: T = {} as any;

This comment has been minimized.

@kitsonk

kitsonk Nov 24, 2017

Member
private _state = {} as T;
@@ -57,15 +57,15 @@ export function walk(segments: string[], object: any, clone = true): PointerTarg
}, pointerTarget);
}
export class Pointer {
export class Pointer<T = any, U = any> {

This comment has been minimized.

@kitsonk

kitsonk Nov 24, 2017

Member

There seems like there is a more direct relationship between a Store and a Pointer?

This comment has been minimized.

@kitsonk

kitsonk Nov 24, 2017

Member

Or at least a relationship between T and U?

This comment has been minimized.

@maier49

maier49 Nov 27, 2017

Contributor

Ideally, but I'm not sure of a good way to express that. If you look at the State interface, where there is P1 = keyof T, P2 = keyof T[P1] etc. U might correspond to any of those signatures. We could have an interface similar to State that iterates all the possibilities, e.g.

interface createPointer {
 <T, P1 extends keyof T>(p1: P1): Pointer<T, T[P1]>;
 <T, P1 extends keyof T, P2 extends keyof T[P1]>(p1: P1, p2: P2): Pointer<T, T[P1][P2]>;
 // ...
}

But even that's tricky. Because we aren't actually passing anything with type T, it would have to be explicitly typed. That ends up with all the subsequent types needing to be passed or just collapsing to their defaults if they're provided.

This comment has been minimized.

@kitsonk
<T>(commands: (Command<T>[] | Command<T>)[], callback?: ProcessCallback<T>): Process<T>;
}
export function createCommandFactory<T>(store?: Store<T>): CommandFactory<T> {

This comment has been minimized.

@agubler

agubler Nov 24, 2017

Member

do you think we should pass the store to the createCommandFactory? Feels like that it would be a pattern we wouldn't want to encourage as commands are purposely decoupled from knowing anything about the store? The user would still be able to provide the T generic to explicitly define the state type.

This comment has been minimized.

@maier49

maier49 Nov 27, 2017

Contributor

That's fine. I put it in there as just a way to avoid having to provide the generic twice, but it makes sense that we are trying to decouple the two.

value: T;
}
export interface State<M> {

This comment has been minimized.

@agubler

agubler Nov 24, 2017

Member

Do you think we should add some jsdoc for interface? (I know generally that could be improved across the repository also...)

@maier49

This comment has been minimized.

Contributor

maier49 commented Nov 27, 2017

@kitsonk @agubler Feedback has been addressed or responded to. The coverage issue is because the class declaration of Store is now showing up as an uncovered branch for some reason.

@@ -35,12 +96,39 @@ export class Store extends Evented {
return patchResult.undoOperations;
}
public at = <U = any>(path: Path<T, Array<U>>, index: number): Path<T, U> => {
const array: any[] = this.get(path);

This comment has been minimized.

@agubler

agubler Nov 28, 2017

Member

is this really an any[]?

This comment has been minimized.

@maier49

maier49 Nov 28, 2017

Contributor

No it's not. The type annotation isn't needed at all. Good catch

segments = [ ...new Pointer(path.path).segments, ...segments ];
}
const pointer = new Pointer(segments.length <= 1 ? (segments[0] || '') : segments.filter<string>(isString));

This comment has been minimized.

@agubler

agubler Nov 28, 2017

Member

This is a but hard to read, can we separate it out?

@maier49

This comment has been minimized.

Contributor

maier49 commented Nov 30, 2017

@agubler Just a reminder I've addressed your last round of feedback in case you have time to take a look again

@maier49 maier49 merged commit b4eca72 into dojo:master Nov 30, 2017

3 of 4 checks passed

codecov/patch 97.29% of diff hit (target 99.46%)
Details
codecov/project 99.5% (+0.03%) compared to bbd4d67
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@maier49 maier49 deleted the maier49:type-store-contents branch Nov 30, 2017

@agubler

This comment has been minimized.

Member

agubler commented Dec 1, 2017

@maier49 Do you think we should add something to the readme?

@dylans dylans added this to the beta.5 milestone Dec 15, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.