-
Notifications
You must be signed in to change notification settings - Fork 114
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
feat: Add getRequest method int IAuthGuard interface #502
feat: Add getRequest method int IAuthGuard interface #502
Conversation
d2ac367
to
d6e5d2d
Compare
lib/auth.guard.ts
Outdated
if (err || !user) { | ||
throw err || new UnauthorizedException(); | ||
} | ||
return user; | ||
return user as any as TUser; | ||
} |
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 please revert back changes unrelated to this PR (move the type argument, generic parameter)? Otherwise, it looks good!
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 your feedback. I did that to be able to compile. I got an error without this change. But I'll try to do something better :)
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.
That should be good now :)
d6e5d2d
to
871bf00
Compare
}; | ||
export const AuthGuard: ( | ||
type?: string | string[] | ||
) => Type<IAuthGuard> = memoize(createAuthGuard); | ||
|
||
const NO_STRATEGY_ERROR = `In order to use "defaultStrategy", please, ensure to import PassportModule in each place where AuthGuard() is being used. Otherwise, passport won't work correctly.`; | ||
|
||
function createAuthGuard(type?: string | string[]): Type<CanActivate> { | ||
function createAuthGuard(type?: string | string[]): Type<IAuthGuard> { |
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.
Is this really necessary? I think adding the type directly in the IAuthGuard type definition is enough.
Good job @ferjul17, I was about to send a similar PR. @kamilmysliwiec have any plans to include this in the next release? |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
Currently I have to override the type returned by
AuthGuard()
otherwise typescript complains becausesuper.getRequest()
does not exist.What is the new behavior?
With these changes, I do not need this "hack" anymore, the type returned by
AuthGuard()
contains the methodgetRequest()
.Does this PR introduce a breaking change?
Other information