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 연동 #63

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
5 changes: 4 additions & 1 deletion src/apis/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { createAxiosHTTPClient } from '@/utils/httpClient';

import AuthApi from './authApi';
import ServiceApi from './serviceApi';
import UserApi from './userApi';
import { createAxiosHTTPClient } from '@/utils/httpClient';

const getApiBaseURL = (): string => {
return process.env.APP_API_URL ?? '/';
Expand All @@ -11,3 +13,4 @@ const client = createAxiosHTTPClient({ baseURL, withCredentials: true });

export const authApi = new AuthApi(client, '/api/auth');
export const userApi = new UserApi(client, '/api/users');
export const serviceApi = new ServiceApi(client, '/api/projects');
17 changes: 17 additions & 0 deletions src/apis/serviceApi/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { ServiceListAndCountResponseBody } from '@/types';

import Api from '../api';

class ServiceApi extends Api {
async getListAndCount(): Promise<ServiceListAndCountResponseBody> {
const response = await this.get<ServiceListAndCountResponseBody>();

return response.data;
}

async deleteItem(id: string): Promise<void> {
await this.delete({ additionalUri: `/${id}` });
}
}

export default ServiceApi;
9 changes: 9 additions & 0 deletions src/components/common/Loader/index.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import React from 'react';

import { render } from '@testing-library/react';

import Loader from '.';

it('Loader rendering test', () => {
render(<Loader />);
});
13 changes: 13 additions & 0 deletions src/components/common/Loader/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React, { FC } from 'react';

import { loaderWrapperStyle, spinnerStyle } from './style';

const Loader: FC = () => {
return (
<div css={loaderWrapperStyle}>
<div css={spinnerStyle} />
</div>
);
};
Seogeurim marked this conversation as resolved.
Show resolved Hide resolved

export default Loader;
30 changes: 30 additions & 0 deletions src/components/common/Loader/style.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { SerializedStyles, Theme, css } from '@emotion/react';
import zIndexes from '@/styles/zIndexes';

export const loaderWrapperStyle = css`
position: fixed;
top: 0;
left: 0;
width: 100%;
height: 100%;
display: flex;
justify-content: center;
align-items: center;
background-color: rgba(0, 0, 0, 0.2);
z-index: ${zIndexes.loader};
`;

export const spinnerStyle = (theme: Theme): SerializedStyles => css`
@keyframes spinner {
0% {
transform: rotate(360deg);
}
}
margin: 0 auto;
width: 30px;
height: 30px;
border-radius: 50%;
border: 6px solid ${theme.colorPrimaryLight};
border-top-color: ${theme.colorPrimary};
animation: spinner 600ms linear infinite;
`;
8 changes: 4 additions & 4 deletions src/components/common/Navbar/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,17 @@ import React, { FC } from 'react';
import { Link } from 'react-router-dom';

import { useUserState } from '@/atoms/userState';
import useConfirmModal from '@/hooks/useConfirmModal';
import Uri from '@/constants/uri';
import Logo from '@/components/common/Logo';
import PrivateNavbar from '@/components/nav/PrivateNavbar';
import PublicNavbar from '@/components/nav/PublicNavbar';
import Logo from '@/components/common/Logo';
import Uri from '@/constants/uri';
import useConfirmModal from '@/hooks/useConfirmModal';

import { navStyle } from './style';

const Navbar: FC = () => {
const [user, setUser] = useUserState();
const [open] = useConfirmModal();
const { open } = useConfirmModal();

const logoutHandler = () => {
const logout = () => setUser(null);
Expand Down
20 changes: 14 additions & 6 deletions src/components/service/ServiceItem/index.test.tsx
Original file line number Diff line number Diff line change
@@ -1,24 +1,32 @@
import React from 'react';
import { RecoilRoot } from 'recoil';

import { render, screen } from '@testing-library/react';

import ServiceItem from '.';

describe('<ServiceItem/> component test', () => {
const id = 1;
const name = 'serviceName';
const domain = 'serviceDomain';
const clientId = 'serviceClientId';

it('서비스 아이템 정보를 렌더링해야 합니다.', () => {
render(<ServiceItem name={name} domain={domain} clientId={clientId} />);
it('서비스 아이템 정보 및 삭제 아이콘을 렌더링해야 합니다.', () => {
render(
<RecoilRoot>
<ServiceItem
id={id}
name={name}
domain={domain}
clientId={clientId}
onDeleteServiceItem={() => {}}
/>
</RecoilRoot>,
);

expect(screen.queryByRole('heading', { level: 3 })).toHaveTextContent(name);
expect(screen.queryByText(domain)).toBeInTheDocument();
expect(screen.queryByText(`Client ID : ${clientId}`)).toBeInTheDocument();
});

it('서비스 아이템 삭제 아이콘을 렌더링해야 합니다.', () => {
render(<ServiceItem name={name} domain={domain} clientId={clientId} />);

expect(screen.getByRole('img', { name: 'remove' })).toBeInTheDocument();
});
Expand Down
19 changes: 17 additions & 2 deletions src/components/service/ServiceItem/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,30 @@ import {
} from './style';

type ServiceItemProps = {
id: number;
name: string;
domain: string;
clientId: string;
onDeleteServiceItem: (id: string) => void;
};

const ServiceItem: FC<ServiceItemProps> = ({ name, domain, clientId }) => {
const [open] = useConfirmModal();
const ServiceItem: FC<ServiceItemProps> = ({
id,
name,
domain,
clientId,
onDeleteServiceItem,
}) => {
const { open, setConfirmModal } = useConfirmModal();

const openModal = () => {
setConfirmModal({
acceptHandler: () => {
onDeleteServiceItem(id.toString());
window.location.reload();
Copy link
Member

Choose a reason for hiding this comment

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

reload 를 하면 해당 페이지 리소스를 다시 불러와야 해서 SPA의 장점을 저해시키지 않을까요...?

새로 API 요청하는 건 어떻게 생각하시나요??

Copy link
Member

@hseoy hseoy Dec 31, 2021

Choose a reason for hiding this comment

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

window.location.reload()와 같은 방식은 종호님 말씀처럼 이미 가지고 있는 리소스까지 다시 요청하기 때문에 불필요하게 과도한 네트워크 비용이 발생하지 않을까 싶어 수정하는 게 좋다고 생각합니다 ㅎㅎ

저는 개인적으로 요런 아이템을 삭제하는 API를 요청하고 성공하면 아이템 리스트 상태(state)로부터 해당 아이템을 지워주는 방식을 선택해왔는 데 요런 방식은 어떤가요?

아이템을 삭제하는 API가 성공했다면 클라이언트 측에서는 해당 아이템이 정상적으로 지워졌다라는 것을 믿는 게 맞지 않을까 했습니다. 실패했는 데 성공 코드를 응답하는 경우가 있다면 해당 경우는 서버 측 예외 처리가 제대로 되지 않았다는 의미일테니 서버 측에서 수정해야 하는 이슈이지 않을까 싶습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

별 생각 없이 reload했는데 이런 부분을 고려하지 못했네요!!! 자세히 리뷰해주시니 이런 부분들을 얻어가네요 감사합니다 ㅎㅎㅎ
API 요청 후 -> 성공 시 -> state로부터 아이템 삭제하고 -> delete success 관련해 snackbar를 띄워주는 방식으로 한 번 고쳐볼게요!!

},
});

open({ message: `Do you really want to delete ${name} service?` });
};

Expand Down
20 changes: 17 additions & 3 deletions src/components/service/ServiceList/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,32 @@ describe('<ServiceList/> component test', () => {
const domain = 'serviceDomain';
const clientId = 'serviceClientId';
const serviceList = [{ id: 1, name, domain, clientId }];
render(<ServiceList serviceList={serviceList} />);
const serviceCount = 1;

render(
<ServiceList
serviceCount={serviceCount}
serviceList={serviceList}
onDeleteServiceItem={() => {}}
/>,
);

expect(screen.getByRole('heading', { level: 2 })).toHaveTextContent(
'Services',
`Services ${serviceCount}`,
Comment on lines -16 to +24
Copy link
Member

Choose a reason for hiding this comment

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

서비스 개수 보여주는 거 좋네요!!

GOOD!!! 👍👍👍

);
expect(screen.queryByRole('heading', { level: 3 })).toHaveTextContent(name);
expect(screen.queryByText(domain)).toBeInTheDocument();
expect(screen.queryByText(`Client ID : ${clientId}`)).toBeInTheDocument();
});

it('serviceList가 빈 배열이면 서비스가 없다는 화면을 렌더링합니다.', () => {
render(<ServiceList serviceList={[]} />);
render(
<ServiceList
serviceCount={0}
serviceList={[]}
onDeleteServiceItem={() => {}}
/>,
);

expect(screen.getByRole('img', { name: 'empty' })).toBeInTheDocument();
expect(screen.queryByText('There are no services')).toBeInTheDocument();
Expand Down
14 changes: 12 additions & 2 deletions src/components/service/ServiceList/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,38 @@ import ServiceItem from '../ServiceItem';
import { emptyStyle, headerStyle, listStyle, wrapperStyle } from './style';

type ServiceListProps = {
serviceCount: number;
serviceList: {
id: number;
name: string;
domain: string | null;
clientId: string;
}[];
onDeleteServiceItem: (id: string) => void;
};

const ServiceList: FC<ServiceListProps> = ({ serviceList }) => {
const ServiceList: FC<ServiceListProps> = ({
serviceCount,
serviceList,
onDeleteServiceItem,
}) => {
return (
<section css={wrapperStyle}>
{serviceList && serviceList.length > 0 ? (
<>
<h2 css={headerStyle}>Services</h2>
<h2 css={headerStyle}>
Services <span>{serviceCount}</span>
</h2>
Comment on lines +28 to +30
Copy link
Member

@hseoy hseoy Dec 31, 2021

Choose a reason for hiding this comment

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

으음..... Emotion과 ClassName을 함께 사용하는 건 어떤가요?? 약간 컨벤션에 대한 얘기가 될 수도 있겠네요.

어떤 emotion css style 하위에 있는 요소를 select할 때 단순히 태그 선택자만 사용하니까 css 코드에서는 이게 어떤 것에 대한 스타일인지 잘 모르겠고 html 코드에서는 여기에 스타일이 적용되어 있는 건지 안되어 있는 건지 잘 모르겠어요.

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 저희가 처음에 마크업 구조를 잘 보기 위해 css로 넣어주길 선택한건데, 이렇게 하다보니 class로 스타일을 지정하는 장점도 뺏기고, styled-component를 사용했을 때의 장점도 뺏기는 느낌이네요 ㅠㅠㅠ
지난번 마크업 PR 올렸을 때도, 어떤 스타일 하위에 h1 선택자로 스타일을 지정하고 있었는데, 이 태그가 h1에서 h2로 바뀌니까 스타일이 갑자기 풀렸는데 어디가 문제인지 바로 인식이 안 되더라구요!!
css 코드가 점점 많아질수록 관리하기 힘들어지는 문제가 있을 것 같습니다. 이 부분 다음 회의 때 컨벤션 다시 한 번 논의해봐도 좋을 것 같네요!

<div css={listStyle}>
{serviceList.map((service) => {
return (
<ServiceItem
key={service.id}
id={service.id}
name={service.name}
domain={service.domain || ''}
clientId={service.clientId}
onDeleteServiceItem={onDeleteServiceItem}
/>
);
})}
Expand Down
4 changes: 4 additions & 0 deletions src/components/service/ServiceList/style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ export const headerStyle = (theme: Theme): SerializedStyles => css`
color: ${theme.colorPrimary};
font-size: 26px;
margin: 30px 0;

span {
font-size: 24px;
}
`;

export const listStyle = css`
Expand Down
14 changes: 7 additions & 7 deletions src/hooks/useConfirmModal.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import {
useSetConfirmModalState,
ConfirmModalStateType,
useSetConfirmModalState,
} from '@/atoms/confirmModalState';

const useConfirmModal = (): readonly [
open: (next?: Partial<ConfirmModalStateType>) => void,
close: (next?: Partial<ConfirmModalStateType>) => void,
setConfirmModal: (next?: Partial<ConfirmModalStateType>) => void,
] => {
const useConfirmModal = (): {
open: (next?: Partial<ConfirmModalStateType>) => void;
close: (next?: Partial<ConfirmModalStateType>) => void;
setConfirmModal: (next?: Partial<ConfirmModalStateType>) => void;
} => {
const setConfirmModalState = useSetConfirmModalState();

const setConfirmModal = (next?: Partial<ConfirmModalStateType>) =>
Expand All @@ -19,7 +19,7 @@ const useConfirmModal = (): readonly [
const close = (next?: Partial<ConfirmModalStateType>) =>
setConfirmModalState((prev) => ({ ...prev, next, isOpen: false }));

return [open, close, setConfirmModal];
return { open, close, setConfirmModal };
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.

배열로 하면 각 순서에 맞게 사용하면서 함수의 이름을 customize 할 수 있지만

객체로 하면 각 함수의 이름을 특정지으면서 제약을 건다는 느낌일까요..? ㅎㅎ

이 modal을 open하는 함수 등의 이름을 굳이 바꿀 필요 없으니.. 차라리 객체인게 나을 수 있지 않을까 하는 저의 개인적인 생각입니다 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

아 이 부분을 제가 훅 사용하면서 open, setConfirmModal만 사용하고 싶었는데 배열로 하면
[open, close, setConfirmModal] = useConfimModal();
이런식으로 불필요한 close 함수도 불러와야 하는 문제가 있어 객체 형식으로 바꿨습니다!!

Copy link
Member

Choose a reason for hiding this comment

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

아 그렇군요 ㅎㅎㅎ

};

export default useConfirmModal;
69 changes: 69 additions & 0 deletions src/hooks/useService.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { useEffect, useState } from 'react';

import { serviceApi } from '@/apis';
import { NETWORK_ERROR, UNEXPECTED_ERROR } from '@/constants/error';
import { ServiceBasicInfo } from '@/types';

const useService = (): {
isLoading: boolean;
error: string;
serviceInfo: { count: number; serviceList: ServiceBasicInfo[] };
deleteService: (id: string) => Promise<void>;
} => {
const [isLoading, setIsLoading] = useState(false);
const [error, setError] = useState<string | null>(null);
const [serviceInfo, setServiceInfo] = useState({
count: 0,
serviceList: [],
});

const getServiceListAndCount = async () => {
try {
setIsLoading(true);

const { count, projectList } = await serviceApi.getListAndCount();
Copy link
Member

Choose a reason for hiding this comment

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

음... 저희 이 부분 recoil selector로 캐싱하는 거 어떻게 생각하시나요 ??

Copy link
Member

Choose a reason for hiding this comment

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

저도 동의합니다 ㅎㅎ

useAuth 도 그렇게 수정해볼까 고민 중인데.. 그렇게 되면 autoLogin 을 어떻게 처리해야 할지 고민중이에요..

그렇지만 service를 가져오는 경우는 selector를 사용하는게 더 좋아보입니다 ㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

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

useRecoilValueLoadable 이라는 훅을 사용하면 해당 상태 값의 상태를 가져올 수 있습니다.

hasValue , loading, hasError 를 사용하면 로딩, 에러 등의 처리를 진행할 수 있습니다 !

Copy link
Member

Choose a reason for hiding this comment

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

@jiyong1 오... auth 는 캐싱 안해도 될것 같은 느낌이 드는데... 아닐까요??

Copy link
Member

@hseoy hseoy Dec 31, 2021

Choose a reason for hiding this comment

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

다들 잊고 있지만 설치되어 있는 패키지, React Query......!

Copy link
Member Author

Choose a reason for hiding this comment

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

오호 제가 이 부분 recoil이랑 react-query 좀 더 살펴보면서 수정해보겠습니다!!!

Copy link
Member

Choose a reason for hiding this comment

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

헐 react query 가 있군요... 좋습니다 😄

setServiceInfo({
count,
serviceList: projectList,
});
} catch (e) {
if (e.response) {
setError(UNEXPECTED_ERROR);
} else {
setError(NETWORK_ERROR);
}
throw e;
} finally {
setIsLoading(false);
}
};

const deleteService = async (id: string) => {
try {
setIsLoading(true);

await serviceApi.deleteItem(id);
} catch (e) {
if (e.response) {
setError(UNEXPECTED_ERROR);
} else {
setError(NETWORK_ERROR);
}
} finally {
setIsLoading(false);
}
};

useEffect(() => {
getServiceListAndCount();
}, []);

return {
isLoading,
error,
serviceInfo,
deleteService,
};
};

export default useService;
Loading