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

refactor: Replace React Context with global object for baseURL management #17 #18

Closed
wants to merge 3 commits into from

Conversation

minai621
Copy link

@minai621 minai621 commented Jun 3, 2024

1️⃣ 어떤 작업을 했나요? (Summary)

기존 코드에 영향을 미치지 않는 변경사항

기존 코드에 영향을 미치는 변경사항

logging-system에 존재하는 Context 코드를 모두 삭제하였기 때문에 Product에 적용된 YLSWrapper를 삭제하여야 합니다.

// before
<YLSWrapper baseURL={baseURL}>
    <App />
<\YLSWrapper>

<App />

window 객체에 등록시 기존의 Context 코드보다 번들링 사이즈를 줄일 수 있고 불필요한 코드를 제거하여 유지보수성이 높아지도록 리팩토링하였습니다.

Before

image

After

image

동작 확인

image

기존 Context 방식과 동일하게 동작합니다.

버그 픽스

2️⃣ 알아두시면 좋아요!

logging-system을 사용하는 프로덕트에서는 window 객체의 YLS_CONFIG에 baseUrl의 타입과 데이터를 등록해야 합니다.

// main.tsx
const baseURL = import.meta.env.VITE_API_YLS_URL;

window.YLS_CONFIG = {
  baseURL,
};

// global.d.ts
declare global {
  interface Window {
    YLS_CONFIG: {
      baseURL: string;
    };
  }
}

3️⃣ 추후 작업

4️⃣ 체크리스트 (Checklist)

  • main 브랜치의 최신 코드를 pull 받았나요?

@Hanna922
Copy link
Member

Hanna922 commented Jun 3, 2024

  1. 전역 변수는 모든 스크립트에서 접근할 수 있기 때문에 Context API에 비해 테스트 및 유지보수가 더 어려워집니다.
  2. 전역 변수는 데이터 흐름을 명확하게 추적하기 어려워집니다.
  3. window 객체는 client에서 쉽게 조작될 수 있기 때문에 안전하지 않습니다.
  4. React의 상태 관리 패턴과 맞지 않습니다. 코드 일관성이 떨어집니다.
  5. 사용처에서 window 객체를 건드려야 하며 작성해야 하는 코드 길이도 길어집니다.

전역 변수가 가지는 장점은 '간단함'인데, YLS는 이미 Context API를 설정해두었습니다.
window 객체가 가지는 위 5가지 단점을 모두 상쇄시킬 만큼의 성능 이점을 보고 있지 않습니다.

@minai621
Copy link
Author

minai621 commented Jun 3, 2024

  1. 전역 변수는 모든 스크립트에서 접근할 수 있기 때문에 Context API에 비해 테스트 및 유지보수가 더 어려워집니다.
  2. 전역 변수는 데이터 흐름을 명확하게 추적하기 어려워집니다.
  3. window 객체는 client에서 쉽게 조작될 수 있기 때문에 안전하지 않습니다.
  4. React의 상태 관리 패턴과 맞지 않습니다. 코드 일관성이 떨어집니다.
  5. 사용처에서 window 객체를 건드려야 하며 작성해야 하는 코드 길이도 길어집니다.

전역 변수가 가지는 장점은 '간단함'인데, YLS는 이미 Context API를 설정해두었습니다. window 객체가 가지는 위 5가지 단점을 모두 상쇄시킬 만큼의 성능 이점을 보고 있지 않습니다.

  • 현재 코드에서 이미 전역 변수를 사용하고 있다. baseUrl을 환경변수에서 사용하는 시점부터 전역변수로 선언됩니다.
    따라서, 이미 선언된 데이터를 Context로 래핑한다고 해서 전역 변수가 오염되지 않습니다. (window 객체가 오염돼 baseUrl이 바뀐다면 마찬가지로 Context의 참조도 변경돼야 합니다.)
  • 마찬가지의 근거로 모든 URL을 상태로 관리하지 않는다. 저희가 프로덕트에서 사용하는 axios의 인스턴스에 일일히 상태를 넘겨주지 않지 않나요?
  • 코드의 길이는 번들링 사이즈 크기의 변경으로도 이미 글로벌 객체를 사용하는 것이 더 적다고 판단할 수 있다고 생각합니다.

결국 제 주장은 이미 window 객체를 사용하는데 Context로 래핑한다고해서 상태의 흐름이 바뀐다거나 하지 않는다고 말씀드리고 싶습니당

@Hanna922
Copy link
Member

Hanna922 commented Jun 3, 2024

사용처에서 사용하는 코드 또한 기존 YLS 사용 방식(LogScreen, LogClick)과 차이점이 존재하며 이전 코드가 더 가독성이 좋다고 생각합니다. 현재 코드는 window 객체를 사용하는 것을 너무나 쉽게 확인 가능하기도 하고요.
18ms 만큼의 차이를 보고자 이루어지는 코드 변경이 적합하지 않다는 판단입니다.

ReactDOM.createRoot(document.getElementById('root')!).render(
  <YLSWrapper baseURL={baseURL}>
    <App />
  </YLSWrapper>
);
window.YLS_CONFIG = {
  baseURL,
};

ReactDOM.createRoot(document.getElementById('root')!).render(<App />);

// vite-env.d.ts
/// <reference types="vite/client" />

declare global {
  interface Window {
    YLS_CONFIG: {
      baseURL: string;
    };
  }
}

@minai621 minai621 changed the title Refactor/#17 refactor: Replace React Context with global object for baseURL management #17 Jun 3, 2024
@minai621 minai621 closed this Jun 3, 2024
@minai621 minai621 deleted the refactor/#17 branch June 3, 2024 05:24
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.

refactor: Replace React Context with global object for baseURL management
2 participants