powaaaaa / techer-me-frontend

https://techer-me-frontend.vercel.app
1 stars 1 forks source link

勝手にコードレビュー #100

Open tosaken1116 opened 7 months ago

tosaken1116 commented 7 months ago

勝手にコードレビューする

tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/app/editProfile/page.tsx#L1-L7

"use client"のスコープが広すぎるかもしれない 必要な部分が出てきたら使うようにして極力使わないことを勧める useStateなどの状態を使うコンポーネントでuse clientを使うと必要なもののみをクライアントサイドでレンダリングできる

tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/app/join/page.tsx#L1-L12

Suspenseを使用する場合はfallbackを設定した方が良い もし全てのコンポーネントにfallbackを設定するのがめんどくさい場合は以下のようにするとfallbackを設定せずに済む


import { Suspense as ReactSuspense,ReactNode } from "react"
import { Loading } from "anywhere"
type Props = {
    children:ReactNode
    fallback?:ReactNode
}
const DefaultFallback =()=>{
    return <Loading />
}
export const Suspense = ({children,fallback = DefaultFallback}:Props)=>{
    return <ReactSuspense fallback={fallback}>{children}</ReactSuspense>
}

デフォルトのfallbackUIを設定しておきfallback引数が渡された場合はそちらを使用する
このようにすることで全てのSuspenseにfallbackを設定する必要がなくなる
なおこのままだとSuspenseが二つ存在(独自のものとReactのもの)するためeslint ruleのeslint-plugin-strict-dependenciesなどを用いてデフォルトのreactのsuspenseをimportできないように制限をかけると良い

https://www.npmjs.com/package/eslint-plugin-strict-dependencies
tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/app/layout.tsx#L16-L20

metadataは簡単にでもいいから変えると良い


export const metadata: Metadata = { 
   title: "techer me", 
   description: "this is application of engineer", 
 }; 
tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/app/layout.tsx#L21-L28

日本向けのアプリケーションである場合

     <html lang="ja"> 

とするのが望ましい

tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/components/pages/EditProfile/index.tsx#L68-L71

app authに関しては各所の中で実行するのではなくsrc/libs/firebaseなどを作り、そこで実行しexportする

import { initializeApp } from "firebase/app";
import {
  FIREBASE_API_KEY,
  FIREBASE_AUTH_DOMAIN,
  FIREBASE_PROJECT_ID,
  FIREBASE_STORAGE_BUCKET,
  FIREBASE_MESSAGING_SENDER_ID,
  FIREBASE_APP_ID,
  FTREBASE_DATABASE_URL,
} from "@/constant/env";

export const firebaseConfig = {
  apiKey: FIREBASE_API_KEY,
  authDomain: FIREBASE_AUTH_DOMAIN,
  databaseURL: FTREBASE_DATABASE_URL,
  projectId: FIREBASE_PROJECT_ID,
  storageBucket: FIREBASE_STORAGE_BUCKET,
  messagingSenderId: FIREBASE_MESSAGING_SENDER_ID,
  appId: FIREBASE_APP_ID,
};

+ const app = initializeApp(firebaseConfig); 
+ export const auth = getAuth(app);
tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/components/pages/EditProfile/index.tsx#L93-L95

backendのurlは環境変数に入れる

もしローカルでもバックエンドが動かせるプロダクトの場合以下のようにすれば汎用的なbackendのurlが使用できる

src/constants/index.ts

export const IS_PROD = process.env.NODE_ENV === "production" // 本番環境かどうか
const PROD_BACKEND_URL = process.env["BACKEND_URL"] ?? ""
// client sideで使用する場合
// const PROD_BACKEND_URL = process.env["NEXT_PUBLIC_BACKEND_URL"] 

export const BACKEND_URL = IS_PROD ? PROD_BACKEND_URL : "http://localhost:8080" 

もしローカルでは動かせないプロダクトの場合は.env.localに設定するのが良い

いずれにしても特定のファイル内でハードコーディングするのはNG

tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/components/pages/EditProfile/index.tsx#L155-L157

Errorをthrowする場合はそのコンポーネントをErrorBoundaryでラップする ErrorBoundaryは独自実装しても良いがnext.js app routerにはデフォルトでErrorBoundaryがある またreact-error-boundaryというライブラリもある

error boundaryよしなに

またapp routerではpage.tsxと同階層にerror.tsxというファイルを作ればErrorがthrowされたときにそのコンポーネントが表示される nextjs のerror handling

ページ単位でエラーを管理したい場合はこの手段が有効

ErrorBoundaryもSuspense同様、libsからラッパーコンポーネントを作ってそれを使用することもできる

<ErrorBoundary fallback={<p>error was occur</p>}>
    <EditProfilePage/>
</ErrorBoundary>
tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/components/pages/EditProfile/index.tsx#L166-L168

console.log()は基本残さない ローカルの確認のみ

eslintのruleで設定できるのでおすすめ

https://eslint.org/docs/latest/rules/no-console

tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/components/pages/EditProfile/index.tsx#L178-L192

意図してるコードがprofileが更新されたらバックエンドにリクエストを送るである場合これはアンチパターン

handleFinishEdit内でリクエストを送る方が丸い

tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/main/src/components/pages/EditProfile/index.tsx

全体的にコード量が多い pageコンポーネントである場合はページ内レイアウト,コンポーネントの配置,pageからしか取得できない変数の取得などが主でロジックが含まれることは少ない

EditProfileコンポーネントを別途作りpageコンポーネントにimportしていく方が綺麗

tosaken1116 commented 7 months ago

nits

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/components/pages/QRExchangePage/index.tsx#L41-L43

AuthorizationのBearerとtokenの間スペース空いてないけどこれ認証とおりますか?

tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/components/pages/TL/hooks/index.ts#L117-L128

現在時刻を取得するutils関数を作るといいかもしれない src/libs/time/getNow.ts



export const getNow =()=>{
    const date = new Date();
    //月の取得
    const month = date.getMonth() + 1;
    //日の取得
    const day = date.getDate();
    //時間の取得ただし一桁の場合は0をつける
    const hournum = date.getHours();
    const hour = hournum < 10 ? `0${hournum}` : hournum;
    //分の取得
    const minutenum = date.getMinutes();
    const minute = minutenum < 10 ? `0${minutenum}` : minutenum;
    return {
        date: `${month}/${day}`,
        time: `${hour}:${minute}`
    }
}
tosaken1116 commented 7 months ago

https://github.com/powaaaaa/techer-me-frontend/blob/2f93809f9673f7984b7c982482abc3db658f102d/src/components/pages/Top/hooks/index.tsx#L98-L105

preventDefaultはhtmlが持つ機能を阻止するための関数でよくformのonSubmitに使用される 本来form内でエンターキーを押すとページのリロードが走るためこれを阻止するためにpreventDefaultが使用される

このpreventDefaultはおそらく意味がない

https://developer.mozilla.org/ja/docs/Web/API/Event/preventDefault