Google oauth #7
Google oauth #7
Conversation
Alankvuong
commented
Feb 20, 2024
Alankvuong
commented
- Set up Google Authentication for app
- Show screen when logged in and basic button for signing out
…el, and typesript. Added a temporary home screen that displays on project first load
…ense in package.json file | Removed the theme in home.tsx b/c it is unnecessary | Removed defaults in tsconfig.json file
README.md
Outdated
| @@ -0,0 +1,45 @@ | |||
| # CRUD Database App | |||
|
|
|||
| This code runs the CRUD Database application for volunteers to interact with, input, and update datbase information relating to the Parking Lot Map and Parking Reform Map. | |||
There was a problem hiding this comment.
| This code runs the CRUD Database application for volunteers to interact with, input, and update datbase information relating to the Parking Lot Map and Parking Reform Map. | |
| This code runs the CRUD Database application for volunteers to interact with, input, and update database information relating to the Parking Lot Map and Parking Reform Map. |
crud-backend/src/index.tsx
Outdated
| @@ -0,0 +1,60 @@ | |||
| import express, { Express, Request, Response, Application } from 'express';`` | |||
| // const express = require("express"); | |||
There was a problem hiding this comment.
When landing a PR, you want to remove all commented out code. It's unclear to future readers why it's there, and over time the code tends to get outdated.
It's a good practice to proactively review your PR before asking for review to look for any problems like this.
crud-backend/src/index.tsx
Outdated
| import cors from "cors"; | ||
|
|
||
|
|
||
| // for env file |
There was a problem hiding this comment.
The code is already self-documenting. Not necessary
| // // fetch the current session | ||
| // const currentSession = supabase.auth.getSession(); | ||
| // if(currentSession) { | ||
| // setSession(currentSession); | ||
| // // setUser(currentSession.) | ||
| // } |
There was a problem hiding this comment.
Ditto on removing commented out code.
crud-backend/src/index.tsx
Outdated
| const supabaseUrl = process.env.SUPABASE_URL ?? ''; | ||
| const supabasePublicAnonKey = process.env.SUPABASE_PUBLIC_ANON_KEY ?? ''; | ||
|
|
||
| console.log(supabaseUrl); | ||
|
|
||
| const supabase = createClient(supabaseUrl, supabasePublicAnonKey, { | ||
| auth: { | ||
| autoRefreshToken: false, | ||
| persistSession: true, | ||
| detectSessionInUrl: false | ||
| } | ||
| }); |
There was a problem hiding this comment.
It's useful to put this behind a helper function like createSupabase. You don't need to access the variables supabaseUrl and supabasePublicAnonKey elsewhere, so that gives you better scoping and better abstracts the code.
crud-frontend/src/App.tsx
Outdated
| import HomePage from "./pages/home/home" | ||
| import { AuthProvider } from "./contexts/AuthContext"; | ||
|
|
||
| // const HomePage = lazy(() => import("./pages/home/home")); |
There was a problem hiding this comment.
Ditto commented out code
| <Suspense fallback={<div>Loading...</div>}> | ||
| <Routes> | ||
| <Route path="/" element={<HomePage />} /> | ||
| <Route path="/logged-in" element={<LoggedIn />} /> |
There was a problem hiding this comment.
Normally the URL wouldn't change when you're logged in to a value like this. Was this for experimentation, or you intend to keep it?
There was a problem hiding this comment.
Thanks, Alan. I'm getting a blank screen when I run npm start. No error messages in the dev console or terminal. I imagine this is from not setting up env vars? But I'm surprised there aren't errors.
Per our video chat, please remove the crud-frontend/ folder because it's not necessary. Although that change would be even better in a dedicated precursor PR to delete crud-backend and move crud-frontend to be top level.
| const supabase = createClient(process.env.REACT_APP_SUPABASE_URL, process.env.REACT_APP_SUPABASE_PUBLIC_ANON_KEY, { | ||
| auth: { |
There was a problem hiding this comment.
Please add instructions to the README about what env vars are necessary, where to find them, and where to set them like an .env file.
There was a problem hiding this comment.
Also this seems to duplicate code from supabaseSetup.tsx?
| const { data, error } = await supabase.auth.signInWithOAuth({ | ||
| provider: 'google', | ||
| options: { | ||
| redirectTo: 'http://localhost:3000/logged-in' |
There was a problem hiding this comment.
This shouldn't redirect
| <> | ||
| <div className="google-login-button"> | ||
| <div id="g_id_onload" | ||
| data-client_id="757897629512-3ltcl0q206uancd3h7jbi63aaj3levud.apps.googleusercontent.com" |
There was a problem hiding this comment.
Normally client IDs are risky to leak in source code and should be set via env vars. What is this client ID corresponding to? Google API client ID?
| @@ -0,0 +1,76 @@ | |||
| import React, { useState, useEffect } from "react"; | |||
There was a problem hiding this comment.
Why call this login/login.tsx rather than login/index.tsx or simply login.tsx?
Also I believe the convention is to use capital letters for the file name, Login.tsx.
| import React, { createContext, useState, useEffect, useContext, ReactNode } from "react"; | ||
| import { createClient, Session, AuthChangeEvent } from "@supabase/supabase-js"; | ||
|
|
||
| const supabase = createClient(process.env.REACT_APP_SUPABASE_URL, process.env.REACT_APP_SUPABASE_PUBLIC_ANON_KEY, { |
| @@ -7,5 +7,6 @@ | |||
| <body> | |||
| <div id="app"></div> | |||
| <script type="module" src="index.tsx"></script> | |||
| <script src="https://accounts.google.com/gsi/client" async></script> | |||
There was a problem hiding this comment.
You auto-inject this via the JS code. What approach did you intend to do?
