Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

damusix
Copy link
Contributor

@damusix damusix commented Aug 9, 2024

Previously would resolve to any, they now pickup the generic and the ref overrides.

Previously would resolve to `any`, they now pickup the generic and the
ref overrides.
@damusix damusix added the types TypeScript type definitions label Aug 9, 2024
@damusix damusix closed this Aug 12, 2024
@damusix damusix reopened this Aug 13, 2024
Comment on lines +306 to +310
export type MergeType<T, U> = {
[K in keyof T]: K extends keyof U
? U[K]
: T[K];
};
Copy link
Contributor

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

Suggested change
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;

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

@damusix damusix Sep 5, 2024

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,
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types TypeScript type definitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants