-
Notifications
You must be signed in to change notification settings - Fork 336
Typings improvments #258
base: master
Are you sure you want to change the base?
Typings improvments #258
Conversation
Any updates on this? |
@ElianCordoba what updates you'd like to see? |
I just wanted to know the state of this PR, because i was thinking on doing it |
Development is IMO currently done. Depends if someone thinks, I can improve something. Feel free to dev if you'd like. |
@aleek |
I've had success with a mapped type for constraints. |
Seems like @ansman won't merge it? |
any chance of getting this PR merged? |
(attributes: any, constraints: Constraints, options?: ValidateOption): any; | ||
validate(attributes: any, constraints: Constraints, options?: ValidateOption): any; | ||
async(attributes: any, constraints: Constraints, options?: AsyncValidateOption): Promise<any>; | ||
single(value: any, constraints: Constraints, options?: ValidateOption): any; |
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.
Shouldn't constraints
on single
be of type ConstraintsTypes
since it doesn't map to a key/name?
} | ||
|
||
export interface Constraints { | ||
[name: string]: ConstraintsTypes; |
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.
NIT: Could we make use of Record
here? export type Constraints = Record<string, ConstraintsTypes>
formatters: any; | ||
|
||
(attributes: any, constraints: Constraints, options?: ValidateOption): any; | ||
validate(attributes: any, constraints: Constraints, options?: ValidateOption): any; |
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 improve the return types on these? Maybe something like,
validate - Record<string, string[]> | undefined>
single - string[] | undefined
?
feffe94
to
40e06a1
Compare
Any chance that this can be merged in? The |
What about adding either this typing or #302's to @DefinitelyTyped ? Who would be up for a PR there @sarangjo @aleek @mauricedoepke ? I could do it but not now... |
Will that override the typings provided by this repo? |
Hm right, good point. After a quick search I would guess one could do a combination of microsoft/TypeScript#25495 and https://stackoverflow.com/questions/40322788/how-to-overwrite-incorrect-typescript-type-definition-installed-via-types-packa {
"compilerOptions": {
"module": "validate.js",
"noImplicitAny": true,
"typeRoots": [
"./node_modules/@types"
]
}
} But this get's really hacky and will probably not be merged by @DefinitelyTyped TL;DR the best way probably is to use one of the forks with the correct types as a dependency instead, as proposed in this thread: https://www.reddit.com/r/typescript/comments/cpe0u1/overriding_a_third_party_librarys_exported_types/ Maybe someone with more time can try what works... |
I added ConstraintTypes and Contraints definition. Nice to have, also I like the intellisense in VSC.
Please review. If you like that, I'll add TypeScript Unit tests to jasmine.
Treat this as Work In Progress - please don't merge yet.