Skip to content

Conversation

@yaag19
Copy link
Contributor

@yaag19 yaag19 commented Jun 13, 2021

개요

작업 사항

  • 다음과 같은 조건에 따른 회원가입 유효성 검사 기능 구현
1. alias 별명(공백X, 중복X, 4~15자, 숫자 + 영문자 조합(첫문자 영문자, 영문자 1개이상 숫자 한 개 이상))
2. password 비밀번호(공백X, 8~16자, 영어 소문자, 대문자, 숫자, 특수 문자 각 1개이상) 한글 포함시 예외
3. name 이름(공백X, 2~17자, 형식(only 한글))
4. email 이메일(공백X, 형식(word + @ +word .word))
5. phone 휴대폰(01[0,1,[6~9]]”-”[3자리, 4자리]”-”[4자리])

기타

  • REST API 배포되면, 서버와 통신하는 기능을 구현할 예정입니다.
  • 전역 state관리는 redux로, 비동기 통신은 redux-saga로 할 예정이었지만,
    계획이 변경됨에 따라 state관리는 context API를 이용하고 서버 통신은 axios로 할 예정입니다 ㅎㅎ

@yaag19 yaag19 requested review from gyofeel and nohtaesang June 13, 2021 09:04
@yaag19 yaag19 self-assigned this Jun 13, 2021
@gyofeel
Copy link
Contributor

gyofeel commented Jun 15, 2021

고생하셨습니다~~
몇 가지 리뷰 드립니다.

  • 클론 받아서 feature2 브랜치에서 패키지 설치(yarn add) 후에 start 커맨드로 실행을 했는데, module not found 오류를 만났네요. 아마 아직 사용되지 않거나 없는 경로를 참조하는 import문들에 대한 정리가 덜 되어서 그런 것 같아요. 제가 뭔가 프로젝트 실행을 다르게 했다면 가이드 부탁드려용.
  • 프로젝트 전반에 파일 참조를 위한 경로를 절대 경로로 접근하기 위해 alias path를 사용하면 좋을 것 같아요. 개발 편의성을 위해..(vue 사용 시에는 webpack config으로 설정한 경험이 있는데 react에서는 webpack config를 eject하는 건 부작용이 있어보이고 create-react-app 모듈 차원에서 처리하는 방법도 있는 것 같네요.)
  • sass는 정의한 디렉토리에 맞춰서 규칙성 있게 적용한 게 보기 좋네요.

SignUp.js

  • 컴포넌트 분리: form 태그 하위에 각 input 요소들을 컴포넌트화해서 데이터를 주입하는 건 어떨까요. 현재는 isColor, Validation, signUpInput 객체가 가진 상태 값이 많아 하나의 상태만 바뀌어도 페이지 전체가 rerendering 될 것으로 보입니다. 성능 상 불리할 수 있으니 컴포넌트로 분리해서 rerendering 관리에 대한 고민을 하면 좋을 것 같아요.
  • 사소할 수 있지만 Validation은 첫 글자가 대문자인 특별한 이유가 있는지 궁금했습니다.
  • validateInput 함수나 signUpInput.phone에 정규식을 적용하는 로직(line:35 ~ 47)은 utility 파일에 분리하여 프로젝트 전반에서 사용될 수 있도록 고민하는 것도 좋을 것 같아요. SignUp 페이지에 대한 의존성을 어떻게 줄일 지 고민이 필요하겠죠. 당장 현재 요구 사항에서는 분리가 필요 없을 수도 있지만...
  • validateInput 함수 내에 alias에 대한 유효성 검사 로직과 pwd에 대한 로직을 서로 분리된 함수로 정의하고 호출하면 좋을 것 같아요.
  • 페이지에 노출되는 문구 텍스트(ex. 아이디는 영문자로 시작하고 ...)등은 페이지 컴포넌트 내부에서 constants로 정의해서 관리하면 좋을 것 같아요.(ALIAS_GUIDE_TEXT: '아이디는 영문자로 시작하고 ...')

gyofeel
gyofeel previously approved these changes Jun 15, 2021
nohtaesang
nohtaesang previously approved these changes Jun 16, 2021
validateInput(name, value);
};

const validateInput = (key, value) => {

Choose a reason for hiding this comment

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

하나의 함수가 맡고 있는 기능이 너무 많은 것 같습니다!
key 에 따라 분리 하면 좋을 것 같아요.

validate하는 함수는 결과만을 return 하면 좋을 것 같아요.
그 결과에 대한 것도 type을 지정하면 좋을 듯 하네요 (는 js를 쓰고 계시는군요 호호)
validate의 결과를 render하는 함수는 분리되어 있으면 좋을 듯 해요!

@yaag19 yaag19 dismissed stale reviews from nohtaesang and gyofeel via 8f5d71a June 20, 2021 11:45
@yaag19
Copy link
Contributor Author

yaag19 commented Jun 20, 2021

  • 프로젝트 시작 시, module not found 오류
    -> .gitignore에 포함시켜논 폴더를 다른데서 import 하고있었지 뭐에요~~ 바로 지웠슴당!

  • 프로젝트 전반에 파일 참조를 위한 경로를 절대 경로로 접근하기 위해 alias path를 사용
    -> 저도 파일 구조를 몇번 바꿔보니까 상대 경로가 을매나 나쁜것인지,, 깨달았네유,, 그래서 절대경로로 지정하긴했지만 alias path 설정은 영,,봐도봐도 모르겠네여ㅎ

  • 컴포넌트 분리: form 태그 하위에 각 input 요소들을 컴포넌트화해서 데이터를 주입. 컴포넌트로 분리해서 rerendering 관리에 대한 고민
    -> 아직 리액트의 리렌더링에 대해 많은 공부가 필요한것 같습니다ㅠㅠ 우선 InputBox에 필요한 데이터들을 props로 내려주었고, React.memo를 이용해서 불필요한 리렌더링을 방지하고자 했습니다. 크롬에서 React DevTools을 이용해서 리렌더링을 하면 하이라이팅 되도록 설정해 놓았는데, React.memo를 하기 전에는 회원가입에 input에 입력을 할 때마다 전체 페이지가 하이라이팅 되었다면, memo를 해준 뒤에는 하이라이팅이 사라지긴 했습니다. 그치만 완전히 이해하고 한 것이 아니라서 정말 잘 된것인지 모르겠습니다..ㅠㅠ

  • 사소할 수 있지만 Validation은 첫 글자가 대문자인 특별한 이유가 있는지 -> 완죠니 오타^_^

  • validateInput 함수나 signUpInput.phone에 정규식을 적용하는 로직(line:35 ~ 47)은 utility 파일에 분리하여 프로젝트 전반에서 사용될 수 있도록 고민. SignUp 페이지에 대한 의존성을 어떻게 줄일 지 고민이 필요.
    -> 이 부분은 아직 이해가 잘 되지 않아서 반영하지 못했습니다! 휴대폰에 정규식을 이용해서 자동으로 하이픈이 생기도록 하는 부분을 따로 분리하는 이유는 추후에 다른 곳에서도 쓰일 가능성을 염두하고 코드를 작성하라는 뜻일까요??

  • validateInput 함수 내에 alias에 대한 유효성 검사 로직과 pwd에 대한 로직을 서로 분리된 함수로 정의하고 호출. validateInput 함수가 맡고 있는 기능이 너무 많아, key 에 따라 분리
    -> 저도 코드를 짜면서 함수가 너무 복잡해진다고 생각은 했는데 key따라 분리할 생각은 왜 못했을까요?? 네??! 두분의 리뷰를 보고 바로 분리했습니당!

  • 페이지에 노출되는 문구 텍스트 페이지 컴포넌트 내부에서 constants로 정의해서 관리
    -> 유효성 문구를 const로 관리하도록 바꾸긴 했는데, 리뷰를 다시 보니 해당 페이지 내부에서 관리하라고 리뷰를 주셨었네요! Wow.. 현재는 constants.js라는 파일내에서 정의하고 import해서 쓰고 있는데 이 부분은 다시 변경하도록 하겠습니다!

  • validate하는 함수는 결과만을 return, validate의 결과를 render하는 함수는 분리되어 있으면 좋을 듯 해요!
    -> 이 부분도 고민을 엄청엄청엄청 많이 했는데 제가 이해한것이 맞는지 모르겠습니다... 유효성 검사하는 부분을 함수로 따로 빼고(aliasTest), 검사결과를 리턴받아서(validateAlias) 해당 결과에 따라 렌더하는 함수를 호출(setIncorrectResult, setCorrectResult) 하는 식으로 방식을 바꿔보았는데 맞을까요??

  • 정규식 결과만을 return 하는 함수

const aliasTest = {
    checkFirstCharacter: (value) =>
      JOIN.ALIAS_FIRST_CHARACTER_REGEX.test(value),

    checkAllowedCharacter: (value) =>
      JOIN.ALIAS_ALLOWED_CHARACTER_TYPE_REGEX.test(value),
  };
  • 결과를 render하는 함수
  const validateAlias = (key, value) => {
    if (!aliasTest.checkFirstCharacter(value)) {
      setIncorrectResult(key, JOIN.ALIAS_GUIDE_START_WITH_ENGLISH);
      return;
    }
    if (!aliasTest.checkAllowedCharacter(value)) {
      setIncorrectResult(key, JOIN.ALIAS_GUIDE_UNION_ENGLISH_AND_NUMBER);
      return;
    } else {
      setCorrectResult(key);
    }
  };
  • 결과에 따른 state값을 set하는 함수들
 const setIncorrectResult = (key, guide) => {
    setValidation((prev) => ({
      ...prev,
      [key]: guide,
    }));
    setIsColor((prev) => ({ ...prev, [key]: true }));
  };

  const setCorrectResult = (key) => {
    setValidation((prev) => ({
      ...prev,
      [key]: JOIN.CORRECT_SIGNUP_INPUT,
    }));
    setIsColor((prev) => ({ ...prev, [key]: false }));
  };

Copy link
Contributor

@gyofeel gyofeel left a comment

Choose a reason for hiding this comment

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

고생 많았어요~~

alias path 관련
craco-alias 모듈 적용된 걸 봤어요. 이 모듈에 대해서 잘모르지만 Webpack 환경에서 적용된다는 설명이 있는 걸 보니 그 맥락은 Webpack에서 적용하는 것과 동일하게 내부적으로 처리되는게 아닐까 싶네요. 링크는 웹팩에서 configure 파일 내에서 적용하는 문서 링크입니다. https://webpack.js.org/configuration/resolve/

rerendering 관련
react와 react hooks를 이용한 rendering 최적화에 대해서는 잘몰라서... 공부를 해보니 재밌네요. React.memo를 사용한 것과 useState의 함수형 업데이트라는 개념을 접했는데 지금 프로젝트에는 모두 적용되어 있는 걸 확인했어요.👍 추가로, 재사용되는 함수에 useCallback을 적용했을때 최적화 효과를 볼 수 있다는 글도 봤는데, 이벤트 리스너 콜백으로 사용되면서 하위 컴포넌트에 props로 전달되는 handleOnChange가 대상이 될 수도 있는 것 같은데. 이 부분에 대한 확신은 없네요.(리액트 고수 태상님이 더 많은 조언을 해주실 수 있을 듯 ㅎㅎㅎ)

정규식 유효성 검사 로직 앱 전체 utils 관리 관련
앱 다른 페이지에서 동일한 유효성 검사를 할땐 constants로 관리되는 정규식을 그냥 가져다 사용하면 될 것 같네요. 무시해주세용.

}
}, [signUpInput.phone]);

const handleOnChange = (name, value) => {

Choose a reason for hiding this comment

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

key 하나 씩 분리되어 있지 않으면, 결합성이 높아져서 나중에 수정하는데 어려울 수도 있어요~
ex) 지역 정보가 추가 되어야 한다, 혹은 두 페이지로 나뉘어져서 (첫 페이지는 id, pw. 두 번째 페이지에서 나머지) 등등...

그렇다고 모든걸 atomic 하게 분리하는게 정답은 아니에요!
가장 적절한 방법을 선택하면 될 듯해요!

Copy link

Choose a reason for hiding this comment

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

한 곳에서 관리하는게 좋은 줄 알고 object형태로 관리하려고 했는데 오히려 더 복잡해 질 수 있군요!

Choose a reason for hiding this comment

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

만약 제가 구현한다면,
SignUpContext 를 하나 만들 것 같아욤~

Copy link

Choose a reason for hiding this comment

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

오호,, context api는 전역으로 값을 관리할때 사용하는 것으로 알고 있는데요. 회원가입 관련 input들을 전역으로 관리하는게 좋기 때문에 사용하라는 것일까요??

Copy link

@nohtaesang nohtaesang Jun 28, 2021

Choose a reason for hiding this comment

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

몇개의 컴포넌트들의 전역일 수 있을듯해요!
ex)

<GlobalContextProvider>
  <Navbar/>
  <Container>
     <SignUpContextProvider>
        <NameInput/>
        <PasswordInput/>
     </SignUpContextProvider>
  </Container>
</GlobalContextProvider>

Copy link

Choose a reason for hiding this comment

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

SignUpContext를 만들어보려고 했는데, 아래와 같이 작성하는게 맞을까요?
signUpInput을 object로 만들지 않고 key별로 분리하고 싶은데 방법을 못찾아서 결국 다시 객체형태로 밖에 사용을 못하겠습니다ㅠㅠ
<SignUpContext.Provider value={signUpContextValue}>에서 객체형태가 아니어도 value에 여러 값을 한번에 담을 수 있는 방법이 있나요??

const signUpInput = {
  alias: '',
  pwd: '',
  pwdCheck: '',
  name: '',
  email: '',
  phone: '',
  handleOnChange: () => {},
};

export const SignUpContext = createContext({ signUpInput });

const SignUpContextProvider = ({ children }) => {

  const [signUpInput, setSignUpInput] = useState({
    alias: '',
    pwd: '',
    pwdCheck: '',
    name: '',
    email: '',
    phone: '',
  });

  const handleOnChange = (e) => {
    const { name, value } = e.target;
    setSignUpInput((prev) => ({ ...prev, [name]: value }));
  };

  const signUpContextValue = { signUpInput, handleOnChange };
/*  const debouncedInput = useDebounce(key, 500); ?? */ 

  useEffect(() => {}, [debouncedInput]);
  return (
    <SignUpContext.Provider value={signUpContextValue}>
      {children}
    </SignUpContext.Provider>
  );
};

export default SignUpContextProvider;

Choose a reason for hiding this comment

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

const [name, setName] = React.useState('')
const [pw,setPw] = React.useState('')


return (
 <SignUpContext.Provider value={
   name,
   setName,
   pw,
   setPw
   }
  >
  {children}
</SignUpContext.Provider> 
)

value에 object를 넣으면 여러가지를 담을 수 있어욤!

Copy link

Choose a reason for hiding this comment

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

그냥 넣으면 되는거였구나,, 🥲

phone: validatePhone,
};

signUpItems

Choose a reason for hiding this comment

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

지금은 문제는 없어보이는데, onChange가 일어날 때 바로 validation체크를 하는게 나중에는 헤비한 로직이 될 수 있어요~
그럴땐, change 일 때는 pending 상태로 취급하고 (vaildation 중... 정도의 state?)
debounce 를 사용해서 일정 시간 입력이 없어졌을 때 validation을 하면 change가 일어날 때 마다 validation 로직이 실행되지 않아서서 좋을 듯 해요~

Copy link

@nohtaesang nohtaesang Jun 28, 2021

Choose a reason for hiding this comment

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

ex) 대충 이런느낌의 코드가 될것같아요

const [emailValidated, setEmailValidated] = React.useState<'success'|'pending'|'error'>('pending')
const [email, setEmail] = React.useState('')
const debouncedEmail = useDebounce(email, 500)

const onChangeEmail = (e) =>{
  setEmail(e.target.value)
  setValidated('pending')
}

useEffect(()=>{

  const resultOfValidateEmail =  setEmailValidated(debouncedEmail)
  setEmailValidated(resultOfValidateEmail) 

},[debouncedEmail])

Copy link

Choose a reason for hiding this comment

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

debounce나 throttle을 들어본 적 있는것 같아요! 적용해보겠습니다~

@@ -0,0 +1,37 @@
export const ALIAS_FIRST_CHARACTER_REGEX = /^[a-zA-Z]/;

Choose a reason for hiding this comment

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

저도 예전엔 이렇게 파일로 구분을 많이 지었었는데요,
지금은 이렇게 하고 있습니다.

NameInputForSignIn.tsx
PasswordInputForSingIn.tsx

이런식으로 파일을 만들고, 그 파일 안에 필요한 const, regex, validation 코드 작성 등을 하는식으로요~

생각보다 범용적인 코드는 작성하기 어렵고, 파일이 많아지면 디펜던시가 다양하게 얽히면서 관리가 어려워 지더라구요.
코드를 중복으로 작성하게 되는것에 저도 알레르기가 있었어서 이런식으로 많이 했었는데,
뭔가 그런 관념에 갇힐 필요는 없는것같아요!

@nohtaesang
Copy link

useMemo, useCallback에 대해서 교필이가 언급한게 있는데,
그게 필요한 부분이 생기면 말씀 드릴게요~
아직까지는 한 컴포넌트에서 함수와 렌더링을 담당하고있어서 큰 필요성은 없어보이는데,
반드시 필요할 때가 생기긴해서욥~!

@nohtaesang
Copy link

form 쪽이 노가다가 많잖아?
그래서 예전에 나도 이거 봤었는데, 다음에 해야지 하고 하다가 잊혀져있었는데...
나도 이번에 로그인 하나 만들어야하는거 있어서 ㅋㅋㅋㅋ 누가 추천해줘서 다시 생각난게 있어
https://react-hook-form.com/
이거로 이번에 만들어보려구

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants