-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: 🐛 accurate auth typings #4523
base: master
Are you sure you want to change the base?
Conversation
Previously would resolve to `any`, they now pickup the generic and the ref overrides.
export type MergeType<T, U> = { | ||
[K in keyof T]: K extends keyof U | ||
? U[K] | ||
: T[K]; | ||
}; |
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're not picking the keys of U, I think we're going to miss some parts unless we do this
export type MergeType<T, U> = { | |
[K in keyof T]: K extends keyof U | |
? U[K] | |
: T[K]; | |
}; | |
export type MergeType<T, U> = { | |
[K in keyof T]: K extends keyof U | |
? U[K] | |
: T[K]; | |
} & U; |
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.
U
would be what you're extending from. Hapi only uses T
, which is ReqRefDefaults
in the latter utility type, so any extra properties you might add in the reference type U
are unused if they're not the same as T
. This makes intersecting unnecessary.
Does it give you any issues?
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.
Nope, but MergeType as a generic type can be abused with a mismatch of keys. If it's not doing an actual merge of keys, maybe it should be named otherwise.
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.
Agreed. What name do you suggest? It should only care about objects related to hapi internal types. Anything else would be unused. It merges from left to right, so perhaps ApplyLeft
?
@@ -27,23 +27,24 @@ export interface AppCredentials { | |||
* User-extensible type for request.auth credentials. | |||
*/ | |||
export interface AuthCredentials< | |||
AuthUser extends object = UserCredentials, |
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'm wondering, what difference does it make to remove all those extends?
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.
Technically you can say that User is a string and hapi will accept 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.
It looks like the types were trying to be prescriptive in "good" practices, should we just stick with what hapi does then?
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 reason I removed it was because, to me, it's like the decorator types where it expects you to pass a function but in reality you can pass anything and it will decorate. I think we should type reality, no? IFAIK, technically, credentials
and artifacts
can be a single primitive value; so can app
, so can user
. Hapi does not internally use any of these values. These are user-defined and I think the user should dictate how they need to build their auth.
Example:
https://github.com/hapijs/basic/blob/master/lib/index.js#L64
https://github.com/hapijs/jwt/blob/master/lib/plugin.js#L225
They're never used and can be anything at runtime.
However, if you think we should communicate that these must be objects, I won't disagree either because it's not wrong. I've never not-used an object in these abstractions.
Previously would resolve to
any
, they now pickup the generic and the ref overrides.