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

[DEV] 이미지 데이터 전송 api #41

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

causyj
Copy link
Collaborator

@causyj causyj commented Oct 17, 2024

Summary

이미지 데이터를 전달하는 api 작업을 했습니다.

Description

  • 성별, 이미지 url, 이메일 주소 값 모두 잘 들어가는 것을 확인했습니다.
  • 전송이 성공적일때 waiting 페이지로 이동합니다.
  • 이메일을 입력하지 않을 경우에 null값을 두고 다음 페이지로 이동합니다.
    -gender 값이 female 일경우에는 1, male일 경우에는 0으로 변환하여 저장하였습니다.

@causyj causyj added the dev New feature or request label Oct 17, 2024
@causyj causyj self-assigned this Oct 17, 2024
@causyj causyj linked an issue Oct 17, 2024 that may be closed by this pull request
src/app/email/page.tsx Outdated Show resolved Hide resolved
Copy link
Member

@ldh019 ldh019 left a comment

Choose a reason for hiding this comment

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

답변만 해주시면 어프루브가! 싸다 싸

1lsang
1lsang previously approved these changes Oct 17, 2024
Copy link
Contributor

@1lsang 1lsang left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~~!

Comment on lines +150 to +152
export default dynamic(() => Promise.resolve(EmailEnterView), {
ssr: false,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

혹시 동적 import를 위해서 사용한 부분일까요?! 왜 이렇게 하셨는지 궁금합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저번 PR 내용에도 있던 부분인데요,
아래의 코드와 같이 토큰을 불러오는 과정에 window객체가 쓰이는데

const storedToken = window.sessionStorage.getItem('accessToken') || '';

이 코드를 useEffect안에 넣는 것보다 밖으로 빼는게 나을 것 같다고 하셔서 밖으로 빼고 저 코드를 같이 썼습니다!

Copy link
Member

@falconlee236 falconlee236 Oct 18, 2024

Choose a reason for hiding this comment

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

계속 의문이였던게 왜 storedToken을 계속 세션 스토리지에서 불러오는지 궁금했습니다.
login/page.tsx 파일을 보시면

  useEffect(() => {
    if (storedToken !== '') {
      fetch(
        `${process.env.NEXT_PUBLIC_CLIENT_ADDRESS}:${process.env.NEXT_PUBLIC_CLIENT_PORT}/api/validate?code=${storedToken}`,
      )
        .then((response) => response.json())
        .then(async (data) => {
          if (data.loginSuccess === false) {
            const reissueResponse = await fetch(
              `${process.env.NEXT_PUBLIC_CLIENT_ADDRESS}:${process.env.NEXT_PUBLIC_CLIENT_PORT}/api/reissue?code=${storedToken}`,
            );
            if (reissueResponse.ok) {
              const reissueData = await reissueResponse.json();
              setInsertToken(reissueData.accessToken);
            } else {
              setInsertToken('');
              setErrorMessage(LOGIN_ERROR_MSG);
              setErrorCheckMessage(LOGIN_ERROR_CHECK_MSG);
              router.push(ROUTE_TYPES.ERROR);
            }
          }
        })
        .catch(() => {
          setInsertToken('');
          setErrorMessage(LOGIN_ERROR_MSG);
          setErrorCheckMessage(LOGIN_ERROR_CHECK_MSG);
          router.push(ROUTE_TYPES.ERROR);
        });
    } else {
      setInsertToken(storedToken);
    }
  }, [

맨 마지막에 storedToken을 atom에 넣기 때문에 이후에는 세션 객체를 직접 접근할 필요가 없어서 lazy loading이 필요없습니다.
혹시 이 storeToken을 담는 대상인 tokenAtom의 값을 추출해보신적이 있으신가요? #21
지금 뒤에 있는 모든 코드가 세션스토리지에서 직접 코드를 가져오는데 만약에 tokenAtom에 값이 정상적으로 있다면 이슈 파서 세션 스토리지에서 코드를 가져오는 부분을 전부 제거하고 lazy loading도 삭제하는 것이 좋아보입니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tokenAtom이 있었군요..!
지금 서버가 꺼져있어서 켜지면 수정하겠습니다!

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 4 to 5
const { searchParams } = new URL(request.url);
const searchCode = searchParams.get('code') || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

토큰을 넘기기 위해서 searchParams를 사용한 것 같은데 이런 방법은 불가능할까요?

Suggested change
const { searchParams } = new URL(request.url);
const searchCode = searchParams.get('code') || '';
const headers = request.headers;
// fetch
const response = await fetch(url, {
method: 'POST',
headers,
body
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

우왓 위 코드로도 잘 작동합니다!
궁금한 점이 있는데요, 그러면 저렇게 작성했을때 기존에 있었던 Authorization부분 코드를 작성하지 않아도 되는 건가요?? 클라이언트에서 헤더에 Authorization부분을 포함했기 때문에 상관이 없는 지 궁금합니다!

Copy link
Contributor

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.

아하 감사합니다!

'Content-Type': 'application/json',
Authorization: `Bearer ${searchCode}`,
},
body: JSON.stringify(body),
Copy link
Contributor

Choose a reason for hiding this comment

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

stringify를 실행하는 이유가 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

JSON.stringify를 안쓰면 에러가 났었는데 찾아보니까 JavaScript의 fetch API나 다른 네트워크 요청 라이브러리는 요청 본문을 문자열로 전송해야 한다고 하네요!
아니면 혹시 다른 방법이 있을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

아아 fetch 메서드에서는 객체를 직접 답을 수 없었네요. 매번 axios를 사용하다보니 잊고있었네요! 그대로 사용하면 될것 같습니다!

Copy link
Member

@falconlee236 falconlee236 left a comment

Choose a reason for hiding this comment

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

상수 관련 리뷰 하나랑 에러 페이지 라우팅 관련 리뷰 하나 했습니다!
그리고 맨 마지막에 lazy loading에 대해서 궁근한 점 남겨드렸습니다

Comment on lines +150 to +152
export default dynamic(() => Promise.resolve(EmailEnterView), {
ssr: false,
});
Copy link
Member

@falconlee236 falconlee236 Oct 18, 2024

Choose a reason for hiding this comment

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

계속 의문이였던게 왜 storedToken을 계속 세션 스토리지에서 불러오는지 궁금했습니다.
login/page.tsx 파일을 보시면

  useEffect(() => {
    if (storedToken !== '') {
      fetch(
        `${process.env.NEXT_PUBLIC_CLIENT_ADDRESS}:${process.env.NEXT_PUBLIC_CLIENT_PORT}/api/validate?code=${storedToken}`,
      )
        .then((response) => response.json())
        .then(async (data) => {
          if (data.loginSuccess === false) {
            const reissueResponse = await fetch(
              `${process.env.NEXT_PUBLIC_CLIENT_ADDRESS}:${process.env.NEXT_PUBLIC_CLIENT_PORT}/api/reissue?code=${storedToken}`,
            );
            if (reissueResponse.ok) {
              const reissueData = await reissueResponse.json();
              setInsertToken(reissueData.accessToken);
            } else {
              setInsertToken('');
              setErrorMessage(LOGIN_ERROR_MSG);
              setErrorCheckMessage(LOGIN_ERROR_CHECK_MSG);
              router.push(ROUTE_TYPES.ERROR);
            }
          }
        })
        .catch(() => {
          setInsertToken('');
          setErrorMessage(LOGIN_ERROR_MSG);
          setErrorCheckMessage(LOGIN_ERROR_CHECK_MSG);
          router.push(ROUTE_TYPES.ERROR);
        });
    } else {
      setInsertToken(storedToken);
    }
  }, [

맨 마지막에 storedToken을 atom에 넣기 때문에 이후에는 세션 객체를 직접 접근할 필요가 없어서 lazy loading이 필요없습니다.
혹시 이 storeToken을 담는 대상인 tokenAtom의 값을 추출해보신적이 있으신가요? #21
지금 뒤에 있는 모든 코드가 세션스토리지에서 직접 코드를 가져오는데 만약에 tokenAtom에 값이 정상적으로 있다면 이슈 파서 세션 스토리지에서 코드를 가져오는 부분을 전부 제거하고 lazy loading도 삭제하는 것이 좋아보입니다.

if (response.status === 200) {
router.push(ROUTE_TYPES.WAITING);
} else {
setErrorMessage(IMG_GENERATED_ERROR_MSG);
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.

어웃 그러네요 감사합니다!

const setErrorMessage = useSetAtom(errorMessageAtom);
const setErrorCheckMessage = useSetAtom(errorCheckMessageAtom);
const storedToken = window.sessionStorage.getItem('accessToken') || '';
const gender = genderString === 'female' ? 1 : 0;
Copy link
Member

Choose a reason for hiding this comment

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

남성 여성을 1과 0으로 구분하지말고 ROUTE_TYPE 처럼 상수로 구현해서 GENDER.MALE, GENDER.FEMALE 형식으로 구현하는 방법에 대해서는 어떻게 생각하시나요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 수정해두었습니다~

@falconlee236 falconlee236 added this to the 1.0.0 milestone Oct 18, 2024
Copy link
Contributor

@yymin1022 yymin1022 left a comment

Choose a reason for hiding this comment

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

요구사항 모두 확인된 것 같은데, Approve해도 되는거 아니에요??

@yymin1022
Copy link
Contributor

@falconlee236 ChangeRequested 걸려있어서 Merge가 블락되어있습니다. 확인부탁드려요!

@falconlee236
Copy link
Member

falconlee236 commented Oct 24, 2024

@falconlee236 ChangeRequested 걸려있어서 Merge가 블락되어있습니다. 확인부탁드려요!

리팩토링할거 남아서 서버켜질때 수정해야합니다 하하 @yymin1022

@causyj
Copy link
Collaborator Author

causyj commented Oct 24, 2024

@falconlee236 ChangeRequested 걸려있어서 Merge가 블락되어있습니다. 확인부탁드려요!

리팩토링할거 남아서 서버켜질때 수정해야합니다 하하 @yymin1022

storedToken 불러오는거는 수정할 페이지들 더 있어서 따로 pr올리려고 이슈 파놨습니당

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DEV] 이메일 전달 API
5 participants