-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added more documentation and types to both namespaced and global functions. #52
base: master
Are you sure you want to change the base?
added more documentation and types to both namespaced and global functions. #52
Conversation
made map types bottom out to string rather than any. made map non optional for namespaced helpers to remove confusion.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this library shouldn't be committed.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not recommended to upload vscode or webstorm configs to packages on npm.
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.
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.
export function useState<TState = any>(storeOrMap: Store<TState> | KnownKeys<TState>[], map?: KnownKeys<TState>[]): RefTypes<TState> { | ||
let store = storeOrMap; | ||
|
||
if (arguments.length === 1) { | ||
map = store as KnownKeys<TState>[]; | ||
store = getStoreFromInstance(); | ||
} | ||
return useMapping(store, null, map, computedState); | ||
return useMapping(store as Store<TState>, null, map, computedState); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for the next appearances of it.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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??
src/wrapper.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the alignment seems a bit different here.
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.