added more documentation and types to both namespaced and global functions.#52
added more documentation and types to both namespaced and global functions.#52HIMISOCOOL wants to merge 4 commits into
Conversation
made map types bottom out to string rather than any. made map non optional for namespaced helpers to remove confusion.
davidmeirlevy
left a comment
davidmeirlevy
left a comment
There was a problem hiding this comment.
Hi!
Thanks for this PR, that's awesome!
I just made a few comments about very small things.
Tag me after you fix them, and I'll merge.
Thanks!
| @@ -0,0 +1,3 @@ | |||
| { | |||
There was a problem hiding this comment.
this library shouldn't be committed.
There was a problem hiding this comment.
I can remove this if you would like the issue is that when a user opens this library vscode will default to the version installed under vscode not under node_modules leading to false positives on type errors etc. at edit time.
The typescript build is still has the final say but it was difficult to make these changes without this setting.
There was a problem hiding this comment.
It's not recommended to upload vscode or webstorm configs to packages on npm.
There was a problem hiding this comment.
you can exclude files from uploading to npm, by adding them to https://docs.npmjs.com/cli/v6/configuring-npm/package-json#files and packing the built solution https://docs.npmjs.com/cli/v6/commands/npm-pack you can publish only the files you want in npm.
| store = getStoreFromInstance(); | ||
| } | ||
| return useMapping(store, null, map, computedState); | ||
| return useMapping(store as Store<TState>, null, map, computedState); |
There was a problem hiding this comment.
Using "as" in our code isn't safe enough.
It would be a problem to deliver it to other users.
There was a problem hiding this comment.
same for the next appearances of it.
There was a problem hiding this comment.
can you explain what the issue with as is for your codebase?
because the field store is a union of two known types this is the only way to narrow the type down without adding significant LOC. It is less type safe to not narrow the type.
There was a problem hiding this comment.
the "getStoreFromInstance" is returning this type. if this function will somehow return a different type, this interface should fail.
There was a problem hiding this comment.
if I explicitly make store Store<TState> | KnownKeys<TState>[]
export function useState<TState = any>(storeOrMap: Store<TState> | KnownKeys<TState>[], map?: KnownKeys<TState>[]): RefTypes<TState> {
let store: Store<TState> | KnownKeys<TState>[] = storeOrMap;
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
if (arguments.length === 1) {
map = store as KnownKeys<TState>[];
store = getStoreFromInstance();
}
return useMapping(store as Store<TState>, null, map, computedState);
}then if getStoreFromInstance(); returns anything other than a Store typescript will not compile, does that work for you??
| export type WrappedStore = { | ||
| createNamespacedHelpers: <TState = any, TGetters = any, TActions = any, TMutations = any>(namespace: string) => NamespacedHelpers<TState, TGetters, TActions, TMutations>; | ||
| useState: <TState = any>(map: KnownKeys<TState>[]) => RefTypes<TState>; | ||
| useGetters: <TGetters = any>(map: KnownKeys<TGetters>[]) => ExtractGetterTypes<TGetters>; |
There was a problem hiding this comment.
the alignment seems a bit different here.
2 participants
added function overloads to give function variants more documentation.
made map types bottom out to string rather than any.
made map non optional for namespaced helpers to remove confusion.
add store type to all utils that take store.
turned off strict bind due to a typing error that is only triggered by typescript 3 (works in typescript 4.2).
added types to wrapStore.