Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

[WIP] feat: add generalized redirect AuthAction #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

offgriddev
Copy link

No description provided.

? onRedirect.whenAuthed
: onRedirect.whenUnauthed

if (!authStateConfig || !authStateConfig.destination) {
Copy link

@prescottprue prescottprue Dec 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!authStateConfig || !authStateConfig.destination) {
if (!authStateConfig?.destination) {

Although that implies and - was the OR intentional before? Or were you intending to do:

if (authStateConfig && !authStateConfig.destination)

So that it is defined, but missing the destination. Thats what the error message seemed to imply anyway

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe I'm misunderstanding since that seems to be what you do below?


if (!authStateConfig || !authStateConfig.destination) {
throw new Error(
`The "destination" in the "onRedirect.whenAuthed" and "onRedirect.whenUnauthed" redirect configs must resolve to a non-empty string`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We could technically make this only call out the offender here instead of both (with a conditional like you did above)

@@ -144,6 +145,39 @@ const withAuthUserTokenSSR =
}
}

if ([whenAuthed, whenUnauthed].includes(AuthAction.REDIRECT)) {
const redirectConfig = onRedirect || getConfig().onRedirect

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice - I like the overridable with local config with fallback to global

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants