[legacy-framework] Add support for Page.redirectAuthenticatedTo to be a function with access to session

[s-r-x]

Hello ^-^
This PR closes blitz-js/legacy-framework#64
redirectAuthenticatedTo can now be also a function. It accepts a context, which includes

type RedirectAuthenticatedToFnCtx = {
  publicData: EmptyPublicData
  session: ClientSession
}

i added a publicData because in my use case i need access to some user metadata on the first render, when session is empty.

What are the changes and their implications?

Bug Checklist

• Integration test added (see test docs if needed)

Feature Checklist

• Integration test added (see test docs if needed)
• Documentation added/updated (submit PR to blitzjs.com repo)

[flybayer]

Awesome, thank you!

i added a publicData because in my use case i need access to some user metadata on the first render, when session is empty.

We can't do this, because it will cause React hydration errors. This is the reason useSession is empty on first render. This is a fundamental law of React that we have to live with. The solution is use Page.suppressFirstRenderFlicker = true to hide the first render.

So remove publicData, and then the last thing is opening a PR to the docs repo to update the docs for this :)

[s-r-x]

@flybayer done
Docs PR

[s-r-x]

@flybayer here is one more addition. redirectAuthenticatedTo now gets called only when session is not isLoading. i think this is a better option instead of letting users to do some extra checks in their code:

if(!session.isLoading) {
  return session.role === 'admin' ? '/admin' : '/';
}

[flybayer]

@s-r-x ah.. I was mistaken — I'm sorry! In this case, since we are redirecting, we don't have to wait for second render.

So we should do like you first had it, except pass publicDataStore.getData() as session instead of both that and useSession result. Like this:

redirectAuthenticateTo({session: publicDataStore.getData()})

And the TS type should be session: PublicData. Because in this case, isLoading is not needed and doesn't have any effect.

Make sense?

[s-r-x]

@flybayer yep

[s-r-x]

@flybayer done.
i also added false as a possible redirectAuthenticatedTo value
possible use case for abstract admin route

Page.redirectAuthenticatedTo = ({ session }) => {
  return session.role === 'admin' ? false : '/401'
}

[flybayer]

Should this be an else if?

[s-r-x]

dont't think so. the second if block would never fire(in case if it's "else if") if redirectAuthenticatedTo is a function. but one of the possible return values from this function is an object, which one we should transform to a string inside the second if block on line 63. correct me if i'm wrong.

[erikshestopal]

@s-r-x I think @flybayer is correct because redirectAuthenticatedTo will be truthy since it's a function and typeof redirectAuthenticatedTo !== "string" will also be true because "function" !== "string".

Probably adding a test case would surface this?

if (typeof redirectAuthenticatedTo === "function") {
  redirectAuthenticatedTo = redirectAuthenticatedTo({session: publicData})
  
}

// `redirectAuthenticatedTo` will be truthy since it's a function
//  and "function" !== "string" will equal true, so the second `if` block will run. 

if (redirectAuthenticatedTo && typeof redirectAuthenticatedTo !== "string") {
  redirectAuthenticatedTo = formatWithValidation(redirectAuthenticatedTo)
}

[flybayer]

Technically the code will work fine as is, but it's better to be an else if from a code readability standpoint. Easier to instantly see that only one of these branches will run.

[s-r-x]

@erikshestopal,
@flybayer

Peek.2021-08-12.08-49.mp4

this is still fine because RedirectError accepts not only a string, but also RouteUrlObject(as far as i understand. at least in video it looks like it does) .
but the second if block is never fire in this case, and tbh i'm not quite sure how important that formatWithValidation call is.

[s-r-x]

@flybayer i simplified the code, made everything immutable

[flybayer]

Awesome, thank you!!

[blitzjs-bot]

Added @s-r-x contributions for code