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

바닐라 문벅스 서버 API 연결 #42

Merged
merged 10 commits into from
Mar 17, 2022
Merged

Conversation

xianeml
Copy link
Collaborator

@xianeml xianeml commented Mar 12, 2022

🔎 개요

바닐라 문벅스 프로젝트에 조회, 등록 API만 연결해둔 상태입니다.
기존 작업 머지된 이후 vue3-vite 프로젝트 바로 시작하려고 합니다!


📝 진행상황

  • [ ]
  • [ ]

🖼 스크린샷

💬 의견

@xianeml xianeml requested a review from InSeong-So March 12, 2022 09:20
Copy link
Member

@InSeong-So InSeong-So left a comment

Choose a reason for hiding this comment

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

스터디를 참여하지 못했을 뿐, 수준은 더 높아진...?😅 혹시 스터디를 참여하는 게 힘을 숨기는 거였던....???

너무 많은 발전이 있었네요! 미현님 첫 번째 PR과 현재 PR을 두고 보면 진짜 괄목상대해요!! 많은 부분에서 고민하고, 찾아보고 이를 적용하고자 노력한 흔적이 보입니다. 고생 많으셨어요!! 얼른 vue로 넘어가요🤗

Comment on lines +5 to +10
export const getMenus = (category: string) => {
return request({
url: `/api/category/${category}/menu`,
method: 'GET',
});
};
Copy link
Member

Choose a reason for hiding this comment

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

확실히 API를 쪼개 놓으니 직관성도 높아지고 응집력도 약해지죠? 너무 마음에 들어요👍

여기서 궁금한 건... 에러 처리는 어떻게 되나요?!

Comment on lines +30 to +32
} catch (e) {
console.error(e);
}
Copy link
Member

Choose a reason for hiding this comment

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

console.error로만 처리하는군요! 만약 에러가 발생해서 undefined가 반환되면 무슨 일이 일어나는 걸까요...?👀 에러 처리는 저도 잘 되지 않는 부분인데, 최근의 리뷰에서는 모든 사항에 적절한 에러와 사용자를 위한 시각화를 적용해보라고 하네요. 이건 다음 스터디 때 같이 이야기하면 좋겠어요🤤

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 default async (config: TrequestConfig) => {
const { url, method, data } = config;

const requestUrl = SERVER_URL + url;
Copy link
Member

Choose a reason for hiding this comment

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

${SERVER_URL}${url}로도 처리할 수 있을 것 같긴 한데... 가독성은 미현님이 작성한 게 더 높아 보이기도 하고...👀 최신기술이 마냥 좋은 게 아니라는 걸 다시 실감합니다🤗

Comment on lines +13 to +29
try {
const response = await fetch(requestUrl, {
method,
headers,
body: JSON.stringify(data),
});

if (response.status !== 200) throw Error('요청 에러');
if (method !== 'GET') return;

const resData = await response.json();

return resData.map((data: TmenuResponse) => ({
id: data.menuId,
menuName: data.name,
inStock: !data.isSoldOut,
})) as Tmenu[];
Copy link
Member

Choose a reason for hiding this comment

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

특정 상황에 throw를 사용하고, early return을 사용하신 점 인상 깊네요! GET이 아닌 경우 return을 한 것은 다른 이유가 있을까요? 저장한 결과 값을 상환 받을 필요가 없어서 이렇게 처리하신 것 같기도...?🙄

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 type Treducer = (state: Tstate, action: TmenuAction) => Promise<Tstate>;

export type Tstore = {
getState: () => Promise<Tstate>;
Copy link
Member

Choose a reason for hiding this comment

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

비동기 통신이 들어가면서 getState가 Promise로 바뀌는군요ㅠ 저는 처음에 이렇게 적용하면서 모든 store 의존 부분을 async/await로 바꿨던 기억이 있네요...🤦‍♂️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네.. 저도 getState 부분에 모두 비동기 처리를 추가했습니다.. 🤦‍♀️
더 나은 방법을 고민해봐야겠어요

Comment on lines +12 to +14
$('nav').addEventListener('click', handleNavigation(store));
$menuForm.addEventListener('submit', handleSubmitMenuForm(store));
$menuList.addEventListener('click', handleMenuList(store));
Copy link
Member

@InSeong-So InSeong-So Mar 14, 2022

Choose a reason for hiding this comment

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

와... 한 번 알려드렸더니 바로 currying 패턴을 사용하시네요. 멋져요!!👍👍

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 7
const render = () => renderViews(store);
render();

// TODO: 리스너 파라미터로 상태를 넘겨주려면?
store.subscribe(render);
Copy link
Member

Choose a reason for hiding this comment

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

API 통신을 추가했다고 이렇게 코드가 간결해지지 않을 텐데, 2월에 무슨 일이???????

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

2월 마지막 세션 때 두분께 실시간 리뷰받고 코드가 훨씬 간결해졌어요 🤩🤩 너무 감사합니다 👍

Comment on lines 38 to 48
if (target.matches('.menu-edit-button')) {
editMenu(targetNodeId);
const menuName = getNewMenuName();
store.dispatch(editMenuItem(targetMenuId, menuName));
} else if (target.matches('.menu-remove-button')) {
removeMenu(targetNodeId);
if (!confirm('메뉴를 삭제하시겠습니까?')) return;
store.dispatch(removeMenuItem(targetMenuId));
} else if (target.matches('.menu-soldout-button')) {
soldOutMenu(targetNodeId);
if (!confirm('메뉴를 품절 처리하시겠습니까?')) return;
store.dispatch(soldOutMenuItem(targetMenuId));
}
};
Copy link
Member

Choose a reason for hiding this comment

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

이런 다중 if문을 처리하는 게 항상 골치입니다. 이 부분은 조건문 캡슐화(조건문마다 메서드로 분리)하거나 객체의 lookup table을 사용해서 처리할 수 있긴 하네요. 저도 최근에 알았기 때문에 이번 스터디 때 이야기 나눠요😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

세션 때 언급해주신 대로 다중 if 문은 고려해야할 조건이 많아서 안티패턴이 될 수 있겠네요 😰
객체 lookup table이 뭔지 몰라서 검색해봤는데 이런걸 조건문 캡슐화라고 하는거군여 ㅎㅎㅎ
새로 배워갑니다, 리팩토링 하면서 적용해봐야겠어요!

Comment on lines +11 to 16
export const handleNavigation = (store: Tstore) => (e: Event) => {
const target = e.target as HTMLElement;
const categoryId = target.dataset['categoryName'] as string;
if (!categoryId) return;
store.dispatch(setCurrentTab(categoryId));
};
Copy link
Member

Choose a reason for hiding this comment

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

curring👍👍 과하게 사용되지 않고 필요한 부분에 쓰였다고 느껴집니다. 잘 작성하셨어요!!

Comment on lines +3 to 9
const MenuCount = ({ menus, currentTab }: Tstate) => {
return `
<h2 class="mt-1">${currentTab.name} 메뉴 관리</h2>
<span id="menu-count" class="mr-2 mt-4 menu-count">
총 ${categoryMenus.length}개
총 ${menus.length}개
</span>`;
};
Copy link
Member

Choose a reason for hiding this comment

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

구조 분해 할당으로 코드를 줄이고 가독성을 높였네요! 멋있어요🤩

@InSeong-So InSeong-So merged commit 568d71e into pagers-org:main Mar 17, 2022
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.

3 participants