-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
refactor(core): allow passing optional check params to Authenticated … #6483
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: ecc5f6b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Hey @reedwane thanks for the quick and neat PR! LGTM 👍🏼 We'll release it in December '24 release. ❤️
check: async ({ params }) => { | ||
// Simulate failed authentication based on params | ||
return params?.allowAuth | ||
? { authenticated: true } | ||
: { authenticated: false, redirectTo: "/login" }; | ||
}, |
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.
Optionally, this can be simplified. Passing a mockCheck
here and assert that mockCheck is called with given mockParams
. But this also works because you are checking if redirect is called, so it's a matter of taste.
Oh, a minor thing I forgot, we could also update |
Oh. Okay @BatuhanW. sadly I have to run now... I'm not sure why the linting is throwing an error... I installed the biome extension, but can't get it to format the file for some reason.... is this something someone else can easily help with? |
…component
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
The
Authenticated
component doesn't take extra params for the useIsAuthenticated hook to check the auth statusWhat is the new behavior?
The component now has
params
in the props which is then passed to the authenticated hook call, which already allows forparams: any
fixes (#6309)
Notes for reviewers