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

[점심 뭐 먹지 미션 Step 2] 센트(김영우) 미션 제출합니다. #81

Merged
merged 7 commits into from
Apr 19, 2023

Conversation

kyw0716
Copy link
Member

@kyw0716 kyw0716 commented Apr 17, 2023

안녕하세요 포코!! 2단계로 다시 찾아왔습니다😀

배포 링크: https://kyw0716.github.io/react-lunch/

남겨주신 리뷰 최대한 반영해보고자 했습니다! 리뷰를 반영하는 중간에 몇 가지 궁금증에 대해 댓글 남겨두었는데 혹시 시간 되시면 확인 부탁드립니다!!

추가적으로 클로저 사용에 관해 궁금한 점이 생겼습니다!!

이번 미션의 도메인 로직을 구성하면서 저는 다음과 같은 함수를 사용했습니다.

export const getRestaurantListObj = () => {
  const list: Restaurant[] = getRestaurantList();

  return {
    getFilteredRestaurant(filter: FilterState) {
      return filterAndSortArray(list, filter);
    },

    getRestaurantInfoById(restaurantId: string) {
      const restaurant = list.find(({ id }) => id === restaurantId);

      if (restaurant === undefined)
        throw new Error(`${restaurantId}를 아이디로 갖는 레스토랑이 없습니다.`);

      return restaurant;
    },
  };
};

처음엔 private field를 사용해 리스트를 보호하는 도메인 로직을 사용하려 했으나 CRA에서 private field를 사용하면 오류가 발생해 클로저를 통해 레스토랑 리스트 정보를 보호해보고자 했습니다. 미션 요구사항에는 리스트에 추가하거나 삭제하는 기능에 대한 부분이 없어 가져오고, id를 통해 탐색하는 것만을 수행하도록 함수를 작성했는데 혹시 이렇게 클로저를 사용하면 예상치 못한 다른 문제가 발생하진 않을지 궁금합니다! 클로저를 기능을 구현하면서 처음 써봤더니 걱정이 됩니다😭😭

@pocojang pocojang self-requested a review April 17, 2023 09:22
@pocojang pocojang self-assigned this Apr 17, 2023
Copy link

@pocojang pocojang left a comment

Choose a reason for hiding this comment

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

@kyw0716 👋

확실히 레벨1에서보다.. 레벨2에서 더 잘하시네요.
리액트에 익숙하신 것 같습니다.

Q&A

Q1. 남겨주신 리뷰 최대한 반영해보고자 했습니다! 리뷰를 반영하는 중간에 몇 가지 궁금증에 대해 댓글 남겨두었는데 혹시 시간 되시면 확인 부탁드립니다!!

몇가지 궁금증은 어디서 볼 수 있을까요...?
못찾겠는데 저에게 알려주세요!

Q2. 추가적으로 클로저 사용에 관해 궁금한 점이 생겼습니다!!

이번 미션의 도메인 로직을 구성하면서 저는 다음과 같은 함수를 사용했습니다.

클로저를 쓰고 싶어서 미션을 하시는건가요...?
미션을 하는데 클로저가 필요하신건가요..?
애초에 private field는 왜 필요한건가요?
리액트에서는 그리고 리액트 어느 문서에도 private field를 사용해달라고 권한 부분이 없을텐데 굳이 왜 사용하려고 애쓰고 고민하시는제 잘 모르겠습니다.

특정 라이브러리나 프레임워크를 사용하기로 마음 먹었다면 최대한 그 룰을 잘 지키셔야합니다.
그리고 마음에 안들때는 PR로 기여하든가 아주 잘 커스텀해야합니다.

지금은.. 내가 해온 개발 방법을 React에 대입하려는 방법에 가까운 것 같네요.
차라리 잘 만든 커스텀 훅을 만들기 위해 노력하는게 좋습니다.

5명 이상의 규모있는 프론트엔드 개발팀에 입사하셔서 getRestaurantListObj() 와 같은 함수를 만들고 공통으로 쓰자고하면 아무도 안쓸 가능성이 큽니다.
공통적인 그라운드 룰과 관심사는 리액트에 맞춰져있기때문이죠!


이런저런 잡소리가 길어졌는데 대충~~ 그렇구나 흘려들으셔도 좋습니다
레벨1~2까지 리뷰해서 즐거웠습니다
레벨2 화이팅!!

list={filterAndSortArray(this.restaurantList, this.state.filter)}
clickRestaurantItem={this.handleClickRestaurantItem.bind(this)}
return (
<Layout>

Choose a reason for hiding this comment

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

오 레이아웃이라는 의미는 좋아요!
다만 다른 네이밍이 필요할 것 같습니다!?!?

Copy link
Member Author

Choose a reason for hiding this comment

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

여러 페이지가 생길 것을 고려해서 여러 곳에 사용할 수 있을 만한 이름을 고민하다가 Layout이란 이름을 선택하게 되었습니다. 한번 정하고 다시 생각해보려니 어떤 이름이 좋을지 잘 떠오르지 않는데 혹시 포코가 생각한 컴포넌트명이 있다면 얘기해주실 수 있나요?!

Choose a reason for hiding this comment

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

아항 아닙니다.
좋은 것 같아요.
요즘 Remix 라는 프레임워크에서 <Outlet />이라는 개념을 도입했고 Next.js에서도 따라하는 상황이에요.
근데 여러분들이 열심히 사용하는.. Router 또한 메인테이닝을 Remix에서 가져가는 중이라 같은 개념을 도입했더라고요.

그래서 말씀드려볼까했는데 제 생각이 잘못된 것 같네요 ㅎㅎ

src/App.tsx Show resolved Hide resolved
src/components/modal/Modal.tsx Show resolved Hide resolved
src/components/modal/RestaurantDetail.tsx Show resolved Hide resolved
src/components/restaurant/RestaurantItem.tsx Show resolved Hide resolved
};
return;
}
window.onkeydown = null;

Choose a reason for hiding this comment

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

이렇게하면 클리어가 잘되나요..? 정말 궁금해서요 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

넵!! 개발자 도구에서 확인해본 결과

모달이 열려있을 때
image

모달이 닫혀있을 때
image

모달을 열면 이벤트 핸들러가 생성되고, 닫으면 이벤트 핸들러가 잘 클리어 됩니다!

Choose a reason for hiding this comment

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

음.. 잘 이벤트가 클리어되고 있지만 useEffect() 클린업을 넣지 않았기때문에 다른 문제는 있을 것 같습니다!

src/domain/storage.ts Show resolved Hide resolved

Choose a reason for hiding this comment

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

간단한 함수인데도 도메인에 의존적인게 아쉽네요 ㅠㅠ

Copy link
Member Author

Choose a reason for hiding this comment

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

도메인 로직을 서버에 api 요청을 넣었을 때 서버에 저장된 정보를 반환하는 것을 모방한 방식으로 코드를 짜려고 하다보니 조금 난해한 코드가 되었던 것 같습니다😭😭

Choose a reason for hiding this comment

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

음.. 지금 앱규모에서는 불필요한 레이어 같기는 하네요

Copy link
Member Author

Choose a reason for hiding this comment

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

처음에 도메인 로직을 짤 때 서버에 레스토랑 리스트 정보가 있고, api 요청을 통해 결과를 받는 것과 같이 제한적으로 해당 리스트에 접근할 수 있게 해보고 싶었습니다. 그래서 리스트에 직접 접근이 불가능하게 private field, 클로져 등등을 사용해보았는데 지금 생각해보니 굳이...? 라는 느낌이 드네요😭😭😭

Choose a reason for hiding this comment

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

후후 울지말아요!
담부터 잘해봅시다

src/components/restaurant/RestaurantList.tsx Show resolved Hide resolved
@pocojang pocojang merged commit 0936229 into woowacourse:kyw0716 Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants