-
Notifications
You must be signed in to change notification settings - Fork 41
feat: authentication user #354
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
Conversation
Visit the preview URL for this PR (updated for commit efd83ba): https://tanam-testing--pr354-issue-340-authentica-nnkwytm6.web.app (expires Mon, 03 Jun 2024 07:44:01 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 5bbe8a1a68d9684d656bffba10008fe532204561 |
import {redirect} from "next/navigation"; | ||
import {SIGN_IN_ROUTE, SESSION_COOKIE_NAME} from "@/constants"; | ||
|
||
export async function createSession(token: string) { |
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.
Could you please link me to the React+Firebase documentation for this 🙏
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.
I haven't read all the documentation on nextjs, I just fixed a little of @detautama PR #346 regarding these changes. Maybe it's not perfect but I would love to read best practices from @detautama regarding these changes
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.
It is Session Management in Server Actions. This is the documentation about Server Actions
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.
Basically, server actions are used for operations on the server side (fetch data, authentication, etc), but in practice they can also be called in the client component
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.
Thank you @detaoddbit what happens if you remove that? I find it peculiar that next requires an additional session handling on top of Firebase Authentication.
@@ -0,0 +1,3 @@ | |||
.bg-pink { | |||
background-color: pink; |
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.
Lol. Please remove this let's just use default background color inherited from global styling.
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.
Lol. Please remove this let's just use default background color inherited from global styling.
Will do.
This file is just for testing sass is loaded in the project or not.
This file is not loaded anywhere. Maybe in the future I'll improve the styling architecture, when I have a chance
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.
Done ✅
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.
Why we need create a component to declare the chidlren
is client-side rendering?
We don't need this component to declare it is client-side rendering. Use use client
is enough in FirebaseUi.tsx
to declare it is client-side rendering. I've been test to run without this component and has no problem in FirebaseUI
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.
Previously, I was still injecting FirebaseUI inside the page (not in a separate component) so use client
couldn't be used inside page
. So I created a ClientOnly
component to tell page
to only display certain parts on the client side. Because the server side does not have a window
context and will get an error when it is read in the server side process.
I would be happy to remove the component if it is not needed and thanks for the sharp eye
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.
Yup that's true, only use client
has access window
context. Please remove this, we already have use client
feature to enable a component render in client-side 🙏🏻
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.
Yup that's true, only
use client
has accesswindow
context. Please remove this, we already haveuse client
feature to enable a component render in client-side 🙏🏻
But if we only add use client
to the component, it will cause an error in the terminal when the server side process is running.
Meanwhile, if we wrap a component that only runs on the client side with the ClientOnly
component, the error does not appear in the terminal when the server side process is running.
So which way will we take? I'm happy to hear other feedback @DennisAlund @detautama
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.
In nuxtjs they already have this component to handle components that are not carried over during the server side build process. https://nuxt.com/docs/api/components/client-only
It seems that the nextjs component that is given use client
only makes the component not appear in the server side rendering process, but the files are still included in the server side build process before it is rendered using server side rendering.
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.
Sometimes nextjs still renders or creates a cache for client components that use use client
on the server, to make it trully client rendering you can make it like what @muzanella11 made or use dynamic import https://nextjs.org/docs/pages/building-your-application/optimizing/lazy-loading
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.
For the latest version of nextjs if you use app
routes it will automatically become SSR if you want to combine it with client side you can use this patterns https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns
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.
Looks good as far as I can understand 🙏
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.
LGTM
What's new:
Before feedback
Screen.Recording.2024-05-26.at.19.45.33.mov
After feedback
Screen.Recording.2024-05-27.at.14.42.13.mov