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

feat: 웹 푸시 구현 #429

Merged
merged 52 commits into from
Oct 13, 2023
Merged

feat: 웹 푸시 구현 #429

merged 52 commits into from
Oct 13, 2023

Conversation

hozzijeong
Copy link
Collaborator

🔥 연관 이슈

🚀 작업 내용

  • FCM을 통해 푸시 알림을 주고 받는다
  • 토글을 통해서 알림을 on/off 할 수 있다.
  • pwa 조건을 맞춘다
  • service worker를 등록한다.
  • 처음 로그인 했을 때 사용자에게 바로가기 설치를 물어본다

💬 리뷰 중점사항

안녕하세요?

끝나고 보니 별거 아니지만 막상 할때는 너무 막막했던 기능입니다 하하

일단 기본적은 구조는 아래와 같습니다.

스크린샷 2023-10-11 오후 6 59 39

클라이언트에서는 서비스 워커를 실행하고, firebase 토큰을 생성하는 역할을 합니다.

클라이언트에서 생성한 FCM 토큰은 알림을 허락하면 백엔드에 post하게 됩니다. 이 FCM 토큰은 DB에 저장되어 있다가 시간이 되었을 때 서버에서 Firebase로 알림을 보내게 됩니다.

여기서 주의할 점이 사용자가 브라우저 알림을 꺼놓는다면 알림을 받을 수 없게 됩니다. 그리고 저희가 코드를 통해서 프롬프트를 띄울 수 없다는 것이 좀 치명적이긴 합니다.

사용자가 브라우저 알림을 설정 했다면 마이 페이지 > 리마인더 알림 받기를 통해 토큰을 등록, 삭제할 수 있습니다. ( 정책상 2달에 한번 토큰 갱신을 해야 한다는데 이건 서버에서 하면 될 것 같아요)

pushManager를 사용해서 subscription을 하지 않고도web-push가 가능한 이유는 firebase를 initial 함으로써 가능하지 않았나 생각이 드네요

삽질을 좀 많이해서 일관적이지 못한 코드가 곳곳에 있을 것 같네요... 리뷰 잘 부탁드립니다!


TODO: 토큰이 자동으로? expire되는데 그 기간을 잘 모르겠더라구요;; 좀 치명적이긴 하네요

Copy link
Member

@bassyu bassyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 클린! 알림 구현하느라 정말 고생 많으셨어요! 👍
코드 리뷰 전에, 전체적인 흐름이 아직 이해가 안되네요 😢 궁금한점 질문 하겠습니다..!
(토글 UI 넘 이뻐요)

클라이언트에서는 서비스 워커를 실행하고, firebase 토큰을 생성하는 역할을 합니다.

firebase는 처음 보는 키워드라 어렵네요.. 잘은 모르지만 서비스 워커에서 알람을 주면 될 것 같은데, 혹시 서비스 워커의 어떤 한계가 있었는지, firebase 어떤 기능이나 장점이 있는지 궁금해요~

클라이언트에서 생성한 FCM 토큰은 알림을 허락하면 백엔드에 post하게 됩니다. 이 FCM 토큰은 DB에 저장되어 있다가 시간이 되었을 때 서버에서 Firebase로 알림을 보내게 됩니다.

firebase를 모르니 이 부분도 이해가 잘 안가네요..
백엔드를 거치지 않고 서비스 워커에서 구독, 시간을 기억해서 cilent - filrebase 둘이서 소통하는 것은 구조적으로 불가능한지, 또는 단점이 있는지 궁금합니다~

import { Navigate, useSearchParams } from 'react-router-dom';
import useLogin from 'hooks/queries/auth/useLogin';
import { URL_PATH } from 'constants/index';

const Authorization = () => {
const [searchParams] = useSearchParams();
const code = searchParams.get('code');
const { mutate } = useLogin();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 어떤가요?

Suggested change
const { mutate } = useLogin();
const { mutate: login } = useLogin();

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋아요!!

Comment on lines +17 to +18
toggleOnCallback,
toggleOffCallback,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동사이기 때문에 함수인걸 알 것 같아서 Callback을 표현 안해줘도 될 것 같은데 어떤가요?
네이밍은 매우 주관적인 것이긴 해서 가볍게 제안해봅니다~

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

함수인 것은 아는데 함수의 역할까지 네이밍에 포함된다고 생각해서 callback을 추가했습니다! toggleOn으로 작성하고서 handler로 사용이 될 수도 있기 때문에 최대한 명시적으로 사용하려고 했습니다

Comment on lines +8 to +9
toggleOnCallback?: () => void;
toggleOffCallback?: () => void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onChange 로 통일해서 !state를 넘겨주는 interface는 어떤지 제안해봅니다!

Suggested change
toggleOnCallback?: () => void;
toggleOffCallback?: () => void;
onChange?: (state: boolean) => void;
<ToggleBtn
        onClick={() => { onChange?.(!state) }}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 toggle에 넘기는 state가 서버에서 받아온 상태인데 그 상태를 임의로 !해서 넘기는 것은 잘 모르겠네요 ㅠㅠ. 저도 Toggle을 작성하면서 범용성이 있어보이지는 않아서 고민이긴 합니다.

현재 구독 정보를 반환하는 메서드에 낙관적 업데이트를 적용해놔서 저희가 직접적으로 상태를 변경시키는 것보다는 인자로 넘어온 것을 사용하는게 좋다고 생각합니다!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 현재 낙관적 업데이트긴 하네요
장바구니 미션의 개수 버튼처럼 토글 클릭 한번에 로딩과 에러를 다 핸들링하는 방향은 어떤가요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 에러는 핸들링 하네요! 꼼꼼하게 못봐서 죄송합니다ㅠ
실패하는 일이 크게 일어날 것 같진 않아서 이대로도 괜찮을 것 같아요!

Copy link
Member

@WaiNaat WaiNaat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 클린~ 완전히 새로운 기능 구현하느라 고생하셨습니다!
알림을 주고받는 부분은.. 일단 돌아간다고 믿고 코드에 관련해서 궁금한 점들 코멘트로 남겼어요

추가로 로컬에서 msw + firebase-sw가 같이 돌아가면 좋겠다는 생각이 들어서 연구한 코드 남겨요
localDevServiceWorker.js 같은 파일을 만들고 안에서 importScripts로 두 서비스워커를 받아온 후 이걸 키면 됩니다

if (process.env.NODE_ENV === 'development') {
  // eslint-disable-next-line @typescript-eslint/no-var-requires
  const { worker } = require('./mocks/browser');
  registerServiceWork('/localDevServiceWorker.js');

  worker.start({
    serviceWorker: {
      url: 'http://localhost:8282/localDevServiceWorker.js',
    },
  });
}

if (process.env.NODE_ENV === 'production') {
  registerServiceWork('/firebase-messaging-sw.js');
}

이거보다 깔끔하게 짤 순 있겠지만 일단 돌쓰 올려봅니다

} else {
toggleOnCallback && toggleOnCallback();
}
}, [state]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R

여기 의존성 배열이 모자라요!
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의존성 배열이 모자란 것은 사실이긴 한데, 결국 Callback이 state의 값에 따라 실행되는 메서드라고 생각해서 의도적으로 추가하지 않았습니다. 혹시 예상되는 문제가 있을까요..?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의존성 배열에 모든 'reactive value'를 넣어야 한다는 건 리액트 공식 문서에서 굉장히 강력하게 말하는 부분이에요. 공식 문서에서는 크게 두 가지 이유를 듭니다

  1. 의존성 배열을 제대로 써야 동기화와 그 해제라는 컨셉을 지킬 수 있어요
  2. 이게 범인인 버그는 찾기/고치기 힘들어요

이펙트 문서에서 이런 설명이 많이 나오지만 useCallback 공식 문서도 이쪽으로 링크를 많이 걸어놨습니다
이렇게까지 본인들이 강하게 주장하는데 린트가 없으면 알려줄 수 있는 방법이 없다는 점은 개인적으론 재밌긴 해요

리액트의 주장을 떠나서 실용주의적인 관점에서 본 제 의견을 말씀드리면 이게 결국엔 리액트에서 기본적으로 제공하는 린트에서 잡아주는 값이고, 많은 CD툴이 린트 경고가 있을 때 빌드를 거부한다는 것과 다음 사람들이 저희 코드를 본다는 점을 고려했을 때 지금부터 의존성 배열을 관리하는 연습을 하는 게 좋다고 생각해요

Notice that you can’t “choose” the dependencies of your Effect. Every reactive value used by your Effect’s code must be declared in your dependency list. The dependency list is determined by the surrounding code:

When dependencies don’t match the code, there is a very high risk of introducing bugs. By suppressing the linter, you “lie” to React about the values your Effect depends on.
Notice that you can’t “choose” the dependencies of your Effect. Every reactive value used by your Effect’s code must be declared in your dependency list. The dependency list is determined by the surrounding code:

When dependencies don’t match the code, there is a very high risk of introducing bugs. By suppressing the linter, you “lie” to React about the values your Effect depends on.

Removing Effect Dependencies
Why is suppressing the dependency linter so dangerous?
#247

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리액트의 주장을 떠나서 실용주의적인 관점에서 본 제 의견을 말씀드리면 이게 결국엔 리액트에서 기본적으로 제공하는 린트에서 잡아주는 값이고, 많은 CD툴이 린트 경고가 있을 때 빌드를 거부한다는 것과 다음 사람들이 저희 코드를 본다는 점을 고려했을 때 지금부터 의존성 배열을 관리하는 연습을 하는 게 좋다고 생각해요

좋습니다! 공식문서를 보니 공감되는 근거가 많네요~ 좋은 자료 공유해주셔서 감사합니다 👍

useLogin(code ?? '');
useEffect(() => {
mutate(code ?? '');
}, []);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R

여기도 의존성 배열이 모자라요
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기는 추가해야겠네요 ㅋㅋㅋㅋ

installAppRef.current.style.display = 'none';
}
const closePrompt = () => {
setDeferredPrompt(null);
};

const beforeInstallPromptHandler = (event: BeforeInstallPromptEvent) => {
event.preventDefault();
const showPrompt = JSON.parse(getCookie('PromptVisible') || 'true');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C

|| 보다 더 뾰족한 ??는 어떠신가요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋아요~~!

};

// TODO: 왜 '/'에서만 beforeinstallprompt가 이벤트 추가가 되나? prompt가 나오고 나머지는 나오지 않는가?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이건 <Main /> 에서만 <InstallPrompt />를 사용해서 그렇습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 주석을 작성했을 때가 처음에 로그인하고 Reminder에서 prompt가 나타나도록 작성했었는데 reminder에서는 나타나지 않고 home에 들어가야지만 나왔을 때 작성한 거였어서... 타임라인 맞추겠습니다..ㅎ

const installAppRef = useRef<HTMLDivElement>(null);
const { isSuccess: isLoggedIn } = useCheckSessionId(false);

const [deferredPrompt, setDeferredPrompt] = useState<BeforeInstallPromptEvent | null>(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C

로그인 상태일 때 이 값이 null이 아니어야 물어보는 것 같은데 피움 첫 접속에 이 값을 설정해주는 곳이 따로 있는건가요? 코드를 강제하지 않는 이상 제 로컬에서는 proppt가 나오지 않아요 😢

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피움 첫 접속 후에 로그인을 했을 때 아래 코드가 실행되면서 값을 할당하도록 의도했습니다!

 const beforeInstallPromptHandler = (event: BeforeInstallPromptEvent) => {
    event.preventDefault();
    const showPrompt = JSON.parse(getCookie('PromptVisible') || 'true');
    if (!showPrompt) return;

    setDeferredPrompt(event);
  };

export const Wrapper = styled.div``;

export const ToggleBtn = styled.button<ToggleParams>`
cursor: pointer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R

disabled일 때 cursor: not-allowed도 부탁드려요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 좋습니다!

retry: noRetryIfUnauthorized,
});

return { subscribe, unSubscribe, currentSubscribe };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C

여기서 돌려주는 값들이 useQuery, useMutation 계열의 훅 결과값인데 이름에서 나타나지 않아서 아쉬워요
사용처가 한정적이고 mutatedata만 사용하니까 여기서 가공해서 보내주는 건 어떠신가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

범용성이나 이해를 위해 반환 값들을 좀 더 직관적으로 작성해 보겠습니다!

badge: './assets/favicon-16x16.png',
data: path,
tag: 'reminder-alert',
vibrate: [200, 100, 200, 100, 200, 100, 200], // 짝수 인덱스는 진동 시간, 홀수 인덱스는 휴식 시간
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이러면 네 번이나 진동하는 건가요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이론상 그렇긴 합니다...

icon: './assets/favicon-32x32.png',
badge: './assets/favicon-16x16.png',
data: path,
tag: 'reminder-alert',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 태그의 역할은 뭔지 궁금해요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 알림의 태그를 의미합니다 여기 알림의 종류가 다양해 질 수 있다고 생각해서 추가했습니다. 나중에 서버에서 보내준 payload에 메세지에 대한 상세 정보가 있을 것이라고 예상해봅니다..

Copy link
Collaborator Author

@hozzijeong hozzijeong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 쵸파 참새?

우선 쵸파가 질문한 것에 대해서 답변을 남겨봅니다!

서비스 워커에서 알람을 주면 될 것 같은데, 혹시 서비스 워커의 어떤 한계가 있었는지, firebase 어떤 기능이나 장점이 있는지 궁금해요~

우선 서비스 워커에서 알람을 주는데, 어떤 종류의 알림을 어느 내용으로 주는지가 명확하지 않습니다. 저희의 경우에는 리마인더 알림을 허용한 사용자의 식물이 오늘 알람을 줘야 하는데 "알림을 허용한" "사용자"의 "식물"에 대한 정보가 서비스 워커에 없기 때문에 해당 정보를 갖고 있는 백엔드가 필요합니다. 만약에 해당 정보를 서비스 워커가 갖고 있다고 한다면 보안 사고라고 생각합니다.

firebase의 역할에 대해서는 너무 많아서 저도 자세히는 설명을 못드리겠네요 ㅠㅠ 하지만 우선은 클라우드 메세지 서비스를 지원하는 것 하나만 알고 있으면 될 것 같습니다!
간단하게는 개발 플랫폼이라고 생각하시면 되는데, 데이터 베이스, 알림, A/B 테스트 GA 등등을 지원하고 있습니다.
지금 저희가 알람 기능을 구현하는게 Web Push를 통해서 기능을 구현하고 있는데 이 Web Push를 구현하기 위해서는 Web Push Protocol을 준수해야 합니다. 여기에서 서버, 클라이언트 그리고 "푸시 서비스"가 필요한데 앞에서 말한 firebase에서 푸시 서비스 기능을 같이 지원하고 있습니다.

왜 firebase를 사용했냐고 물어보시는 것이라면 문서화가 잘 되어 있어서 러닝 커브가 낮고 푸시 서비스 뿐만 아니라 GA와 연동도 되기 때문입니다!!

백엔드를 거치지 않고 서비스 워커에서 구독, 시간을 기억해서 cilent - filrebase 둘이서 소통하는 것은 구조적으로 불가능한지, 또는 단점이 있는지 궁금합니다~

앞에서 말씀드린 protocol이 있어서 불가능하고, 전송할 데이터 역시 클라이언트에 없어서 불가능합니다! 그리고 서비스 워커의 lifeCycle이 존재하는데, 만약에 버전이 업데이트 된다고 하면 데이터가 업데이트 돼야 하는데 그 과정에서 데이터가 유실될 수도 있습니다. 또한 사용자의 정보를 다루는 것이기 때문에 보안에도 문제가 있고 firebase에서 제공하는 token을 유지하려면 살아있는 서버에서 관리해주는게 보안상 더 좋습니다. 그래서 중간에 서버가 필수로 필요합니다!

참새가 말씀하신 serviceWorker와 firebase의 합작품은 한번 츄라이 해보겠습니다.

icon: './assets/favicon-32x32.png',
badge: './assets/favicon-16x16.png',
data: path,
tag: 'reminder-alert',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 알림의 태그를 의미합니다 여기 알림의 종류가 다양해 질 수 있다고 생각해서 추가했습니다. 나중에 서버에서 보내준 payload에 메세지에 대한 상세 정보가 있을 것이라고 예상해봅니다..

badge: './assets/favicon-16x16.png',
data: path,
tag: 'reminder-alert',
vibrate: [200, 100, 200, 100, 200, 100, 200], // 짝수 인덱스는 진동 시간, 홀수 인덱스는 휴식 시간
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이론상 그렇긴 합니다...

const installAppRef = useRef<HTMLDivElement>(null);
const { isSuccess: isLoggedIn } = useCheckSessionId(false);

const [deferredPrompt, setDeferredPrompt] = useState<BeforeInstallPromptEvent | null>(null);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피움 첫 접속 후에 로그인을 했을 때 아래 코드가 실행되면서 값을 할당하도록 의도했습니다!

 const beforeInstallPromptHandler = (event: BeforeInstallPromptEvent) => {
    event.preventDefault();
    const showPrompt = JSON.parse(getCookie('PromptVisible') || 'true');
    if (!showPrompt) return;

    setDeferredPrompt(event);
  };

retry: noRetryIfUnauthorized,
});

return { subscribe, unSubscribe, currentSubscribe };
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

범용성이나 이해를 위해 반환 값들을 좀 더 직관적으로 작성해 보겠습니다!

export const Wrapper = styled.div``;

export const ToggleBtn = styled.button<ToggleParams>`
cursor: pointer;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 좋습니다!

} else {
toggleOnCallback && toggleOnCallback();
}
}, [state]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의존성 배열이 모자란 것은 사실이긴 한데, 결국 Callback이 state의 값에 따라 실행되는 메서드라고 생각해서 의도적으로 추가하지 않았습니다. 혹시 예상되는 문제가 있을까요..?

installAppRef.current.style.display = 'none';
}
const closePrompt = () => {
setDeferredPrompt(null);
};

const beforeInstallPromptHandler = (event: BeforeInstallPromptEvent) => {
event.preventDefault();
const showPrompt = JSON.parse(getCookie('PromptVisible') || 'true');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋아요~~!

};

// TODO: 왜 '/'에서만 beforeinstallprompt가 이벤트 추가가 되나? prompt가 나오고 나머지는 나오지 않는가?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 주석을 작성했을 때가 처음에 로그인하고 Reminder에서 prompt가 나타나도록 작성했었는데 reminder에서는 나타나지 않고 home에 들어가야지만 나왔을 때 작성한 거였어서... 타임라인 맞추겠습니다..ㅎ

import { Navigate, useSearchParams } from 'react-router-dom';
import useLogin from 'hooks/queries/auth/useLogin';
import { URL_PATH } from 'constants/index';

const Authorization = () => {
const [searchParams] = useSearchParams();
const code = searchParams.get('code');
const { mutate } = useLogin();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋아요!!

useLogin(code ?? '');
useEffect(() => {
mutate(code ?? '');
}, []);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기는 추가해야겠네요 ㅋㅋㅋㅋ

Copy link
Member

@bassyu bassyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금 저희가 알람 기능을 구현하는게 Web Push를 통해서 기능을 구현하고 있는데 이 Web Push를 구현하기 위해서는 Web Push Protocol을 준수해야 합니다. 여기에서 서버, 클라이언트 그리고 "푸시 서비스"가 필요한데 앞에서 말한 firebase에서 푸시 서비스 기능을 같이 지원하고 있습니다.

앞에서 말씀드린 protocol이 있어서 불가능하고, 전송할 데이터 역시 클라이언트에 없어서 불가능합니다!

질문을 조금 드렸는데도 자세하게 설명해주셔서 전체적인 흐름이 잘 이해됐어요! 감사합니다 👍👍

Comment on lines +8 to +9
toggleOnCallback?: () => void;
toggleOffCallback?: () => void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 현재 낙관적 업데이트긴 하네요
장바구니 미션의 개수 버튼처럼 토글 클릭 한번에 로딩과 에러를 다 핸들링하는 방향은 어떤가요?

Copy link
Member

@WaiNaat WaiNaat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변경하신 부분들 확인했습니다~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 성공하셨군요

} else {
toggleOnCallback && toggleOnCallback();
}
}, [state]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의존성 배열에 모든 'reactive value'를 넣어야 한다는 건 리액트 공식 문서에서 굉장히 강력하게 말하는 부분이에요. 공식 문서에서는 크게 두 가지 이유를 듭니다

  1. 의존성 배열을 제대로 써야 동기화와 그 해제라는 컨셉을 지킬 수 있어요
  2. 이게 범인인 버그는 찾기/고치기 힘들어요

이펙트 문서에서 이런 설명이 많이 나오지만 useCallback 공식 문서도 이쪽으로 링크를 많이 걸어놨습니다
이렇게까지 본인들이 강하게 주장하는데 린트가 없으면 알려줄 수 있는 방법이 없다는 점은 개인적으론 재밌긴 해요

리액트의 주장을 떠나서 실용주의적인 관점에서 본 제 의견을 말씀드리면 이게 결국엔 리액트에서 기본적으로 제공하는 린트에서 잡아주는 값이고, 많은 CD툴이 린트 경고가 있을 때 빌드를 거부한다는 것과 다음 사람들이 저희 코드를 본다는 점을 고려했을 때 지금부터 의존성 배열을 관리하는 연습을 하는 게 좋다고 생각해요

Notice that you can’t “choose” the dependencies of your Effect. Every reactive value used by your Effect’s code must be declared in your dependency list. The dependency list is determined by the surrounding code:

When dependencies don’t match the code, there is a very high risk of introducing bugs. By suppressing the linter, you “lie” to React about the values your Effect depends on.
Notice that you can’t “choose” the dependencies of your Effect. Every reactive value used by your Effect’s code must be declared in your dependency list. The dependency list is determined by the surrounding code:

When dependencies don’t match the code, there is a very high risk of introducing bugs. By suppressing the linter, you “lie” to React about the values your Effect depends on.

Removing Effect Dependencies
Why is suppressing the dependency linter so dangerous?
#247

Copy link
Member

@WaiNaat WaiNaat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

멋있어요
개발서버에서 잘 됐으면 좋겠네요

@WaiNaat WaiNaat merged commit 9d10fe1 into develop Oct 13, 2023
2 checks passed
@WaiNaat WaiNaat deleted the feature/415-webpush branch October 13, 2023 04:25
Choi-JJunho pushed a commit that referenced this pull request Oct 20, 2023
* chore: manifest 수정

* feat: Toggle Component 구현

* refactor: serviceWorker 등록 코드 변경

* feat: myPage에서 서비스 구독 알림 설정

* chore: firebase 설치

* chore: serviceWorker 배포파일 추가

* chore: firebase 위치 이동

* chore: firebase 이동

* feat: 구독 정보 객체 선언

* feat: webpush 관련 api 작성

* feat: 구동 설정/해제 구현

* refactor: serviceWorker refactor

* refactor: 로그인 하지 않았을 경우 Prompt 나타나지 않게 설정 및 prompt 조건부 렌더링으로 변경

* refactor: disabled인 경우 디자인 변경 및 toggle 초기값 설정

* chore: icon maskable로 변경

* refactor: deferredPrompt 상태로 저장

* chore: serviceWorker에서 firebase-messaging으로 변경

* design: prompt z-index 설정

* refactor: 로그인 시에 firebase 토큰 전달하도록 설정

* chore: firebase 알림 수정

* refactor: 토큰 등록 관련 API 수정

* refactor: webpush 훅 분리

* feat: 토큰 조회 API 구현

* refactor: 사용하지 않는 객체 삭제

* feat: 브라우저에서 알림을 지원하지 않는 경우 옵션 추가

* refactor: 로그인 기능 롤백

* refactor: 현재 구독중인지 확인하는 훅 추가

* chore: web push 관련 handler 생성

* refactor: webpush 관련 훅 업데이트

* refactor: 개발 환경에서 development 생성

* refactor: toggle 낙관적 업데이트 적용

* chore: 불필요한 콘솔 제거

* chore: sitemap 변경

* feat: 코드가 200번대가 아닌 경우에 일단 에러 발생

* refactor: 미지원 브라우저 알림 토글 차단

* refactor: 로그인 msw 롤백

* chore: 사용하지 않는 파일 제거

* chore: submodule update 추가

* chore: build를 위한 주석 테스트

* refactor: build 문제 해결을 위한 문제 코드 삭제

* chore: 주석 제거

* refactor: toast 변경

* chore: pushManager관련 주석 추가

* refactor: login 코드 수정

* refactor: 반환값 명시

* design: disable인 경우에 cursor 제거

* refactor: cookie 값 없는 경우 반환 식 변경

* refactor: 아이콘 이미지 변경

* refactor: serviceWorker 병형 가능하도록 설정

* refactor: 의존성 업데이트

* test: main에 테스트 수정
hozzijeong added a commit that referenced this pull request Oct 20, 2023
* chore: manifest 수정

* feat: Toggle Component 구현

* refactor: serviceWorker 등록 코드 변경

* feat: myPage에서 서비스 구독 알림 설정

* chore: firebase 설치

* chore: serviceWorker 배포파일 추가

* chore: firebase 위치 이동

* chore: firebase 이동

* feat: 구독 정보 객체 선언

* feat: webpush 관련 api 작성

* feat: 구동 설정/해제 구현

* refactor: serviceWorker refactor

* refactor: 로그인 하지 않았을 경우 Prompt 나타나지 않게 설정 및 prompt 조건부 렌더링으로 변경

* refactor: disabled인 경우 디자인 변경 및 toggle 초기값 설정

* chore: icon maskable로 변경

* refactor: deferredPrompt 상태로 저장

* chore: serviceWorker에서 firebase-messaging으로 변경

* design: prompt z-index 설정

* refactor: 로그인 시에 firebase 토큰 전달하도록 설정

* chore: firebase 알림 수정

* refactor: 토큰 등록 관련 API 수정

* refactor: webpush 훅 분리

* feat: 토큰 조회 API 구현

* refactor: 사용하지 않는 객체 삭제

* feat: 브라우저에서 알림을 지원하지 않는 경우 옵션 추가

* refactor: 로그인 기능 롤백

* refactor: 현재 구독중인지 확인하는 훅 추가

* chore: web push 관련 handler 생성

* refactor: webpush 관련 훅 업데이트

* refactor: 개발 환경에서 development 생성

* refactor: toggle 낙관적 업데이트 적용

* chore: 불필요한 콘솔 제거

* chore: sitemap 변경

* feat: 코드가 200번대가 아닌 경우에 일단 에러 발생

* refactor: 미지원 브라우저 알림 토글 차단

* refactor: 로그인 msw 롤백

* chore: 사용하지 않는 파일 제거

* chore: submodule update 추가

* chore: build를 위한 주석 테스트

* refactor: build 문제 해결을 위한 문제 코드 삭제

* chore: 주석 제거

* refactor: toast 변경

* chore: pushManager관련 주석 추가

* refactor: login 코드 수정

* refactor: 반환값 명시

* design: disable인 경우에 cursor 제거

* refactor: cookie 값 없는 경우 반환 식 변경

* refactor: 아이콘 이미지 변경

* refactor: serviceWorker 병형 가능하도록 설정

* refactor: 의존성 업데이트

* test: main에 테스트 수정
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌈 기능 새로운 기능을 개발합니다 🍇 프론트엔드 프론트엔드 관련 이슈입니다
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

웹 푸시 알림을 설정한다
3 participants