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

[legacy-framework] Add authenticated layout support with Layout.authenticate #2825

Merged
merged 8 commits into from
Oct 15, 2021

Conversation

JuanM04
Copy link

@JuanM04 JuanM04 commented Oct 8, 2021

Closes: blitz-js/legacy-framework#349

What are the changes and their implications?

It allows for authenticated layouts. It searches the React tree returned by getLayout and uses the first component with either authenticate or redirectAuthenticatedTo as the default. This value can be overwritten in a per-page basis with the already implemented Page.authenticate

Also, the layout is automatically rendered by BlitzInnerRoot, because it can lead to unwanted behavior

Feature Checklist

@flybayer
Copy link
Member

Wow this looks really great! Just need a docs PR now

@flybayer
Copy link
Member

One question: what happens if user still has their own getLayout() call inside their _app? Will it continue to render fine, just without layout authenticated support?

@JuanM04
Copy link
Author

JuanM04 commented Oct 14, 2021

what happens if user still has their own getLayout() call inside their _app?

@flybayer It will render the Layout twice, one inside the other. So, the changelog must acknowledge this and tell people to strip it from their _app

@JuanM04
Copy link
Author

JuanM04 commented Oct 14, 2021

I figured out a problem I didn't know until now. With this setup, the layout re-renders when the page changes. However, if I remove the getLayout inside BlitzInnerRoot and move it back inside _app, it will work like before, but it will be rendered no matter the state of Layout.authenticate. Should I keep the old behavior and add a note in the docs, or do you have another solution?

@flybayer
Copy link
Member

but it will be rendered no matter the state of Layout.authenticate

What do you mean by this?

@JuanM04
Copy link
Author

JuanM04 commented Oct 14, 2021

For example, if there is a query inside that layout, it will run even if Layout.authenticate = true and the user isn't authenticated. This could lead to unexpected behavior, like throws of UnauthenticatedErrors

@flybayer
Copy link
Member

flybayer commented Oct 14, 2021

@JuanM04 ok so essentially with that approach it's not really adding any functionality at all, right?

Can we expose some helper function like this to make it fully work?

// _app.tsx
import {composeLayouts} from 'blitz'

export default function App({Component, pageProps}: AppProps) {
-  const getLayout = Component.getLayout || ((page) => page)
+  const renderWithLayout = composeLayouts(Component)

Not sure that's the best name, but you get the idea.

@JuanM04 JuanM04 changed the title Authenticated Layouts Secure Layouts Oct 15, 2021
@JuanM04
Copy link
Author

JuanM04 commented Oct 15, 2021

@flybayer sorry, I got confused and it's hard to me to explain technical issues. Yes, this add functionality.

However, there is an edge case (that existed before; it wasn't introduced by this PR):

const Layout = ({children}) => {
  console.log('Log from Layout')
  return <div>{children}</div>
}

const Page = () => {
  console.log('Log from Page')
  return <h1>Authenticated Page </h1>
}

Page.authenticated = true
Page.getLayout = (page) => <Layout>{page}</Layout>

// logs from someone authenticated:
// > Log from Layout
// > Log from Page

// logs from someone unauthenticated:
// > Log from Layout

When someone unauthenticated loads that page, the code inside Page doesn't run while the code inside Layout does. I wanted to mention this somewhere since writing Layout.authenticated = true may seem like it will behave exactly as Page.authenticated = true but it doesn't.

(Note: forget what I said about the breaking changes, I updated the PR so you still need to declare const getLayout = ... inside _app)

@flybayer flybayer changed the title Secure Layouts Add authenticated layout support with Layout.authenticate Oct 15, 2021
@flybayer flybayer merged commit 1457ec8 into canary Oct 15, 2021
@flybayer flybayer deleted the layout-authenticate branch October 15, 2021 15:02
@itsdillon itsdillon changed the title Add authenticated layout support with Layout.authenticate [legacy-framework] Add authenticated layout support with Layout.authenticate Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Declare authenticated pages in blitz.config.js
3 participants