refactor: change RAP interface design #211

wplong11 · 2021-09-12T14:08:12Z

Description

#209 모듈 분리 논의에 앞서 RAP 인터페이스 디자인을 변경합니다.

클라이언트 관점에서(라이브러리를 사용하는 관점) 가장 큰 변화는 AnalyticsProvider 사용 방법이 달라졌다는 점입니다. 그 외에 Product Analytics(이하 PA) 별 지원(확장?) 모듈 구현 방식에 영향을 미치는 변화가 있으며 몇몇 함수와 인터페이스 이름이 바뀌었습니다.

커밋 단위로 확인해주세요. 커밋은 작게 쪼갰으나 PR이 큽니다. 작업물 병합 목적이라기 보다는, 생각하는 방향을 보여주기 위한 목적으로 작성된 PR이기 때문입니다.

작업 내용

AnalyticsProviderContext 인터페이스의 역할과 메서드 이름을 변경합니다. 인터페이스 이름도 Analytics로 변경했고 core 폴더로 이동시켰습니다.
- ProviderContext라는 표현이 불필요해 보여서 지웠습니다. 리액트에서 많이 쓰는 관례인가요? 저는 리액트를 잘 모릅니다 🙏
- Analytics 인터페이스와 구현체는 리액트 의존이 없기 때문에 Svelte, Angular, Vue.js 어디서든 쓸 수 있습니다.
- PA 별 지원 모듈은 Analytics 인터페이스의 구현체를 제공하면 됩니다.
```
class GoogleAnalytics implements Analytics { /* ... */ }
class AmplitudeAnalytics implements Analytics { /* ... */ }
class FirebaseAnalytics implements Analytics { /* ... */ }
class AppsFlyerAnalytics implements Analytics { /* ... */ }
```
useAnalyticsContext 이름을 useAnalytics 으로 변경합니다.
- Context라는 표현이 불필요해 보여서 지웠습니다. 리액트에서 많이 쓰는 관례인가요? 저는 리액트를 잘 모릅니다 🙏
- 이제 다음과 같이 쓸 수 있습니다
```
- const analytics: AnalyticsProviderContext = useAnalyticsContext()
+ const analytics: Analytics = useAnalytics()
```

AnalyticsProvider의 Props 구조를 변경합니다.

analytics와 child node만을 가지도록 수정했습니다.

다음과 같이 PA 지원 모듈이 제공한 구현체를 Composite 패턴으로 조립해서 사용하면 편리할 것 같습니다.

const analytics: Analytics = new CompositeAnalytics( // core가 제공하는 클래스
    new GoogleAnalytics(), // google analytics 지원 모듈이 제공하는 클래스(네이밍이 조금 아쉽네요)
    new AmplitudeAnalytics(), // amplitude 지원 모듈이 제공하는 클래스
    new FirebaseAnalytics(), // firebase 지원 모듈이 제공하는 클래스
    new AppsFlyerAnalytics(), // appsflyer 지원 모듈이 제공하는 클래스
    new DebugAnalytics(), // 콘솔 로그를 남기거나 Toast를 표시하는 클래스. core가 제공하거나 라이브러리 사용자가 직접 정의해도 됨
)
<AnalyticsProvider analytics={analytics}>

Help Wanted 👀

next.js 데모 앱 작동을 확인하지 못 했습니다. 빌드가 안 되네요. 루트 디렉토리에서 npm install && npm run serve를 실행한 뒤에, next.js 데모 프로젝트 디렉토리에서 npm install && npm run build를 실행했습니다. 그랬더니 다음과 같은 오류가 뜹니다 😢

ReferenceError: window is not defined
    at /Users/kimdonghyeon/repos/EveryAnalytics/react-analytics-provider/node_modules/amplitude-js/amplitude.umd.js:1197:12
    at /Users/kimdonghyeon/repos/EveryAnalytics/react-analytics-provider/node_modules/amplitude-js/amplitude.umd.js:2:83
    at Object.<anonymous> (/Users/kimdonghyeon/repos/EveryAnalytics/react-analytics-provider/node_modules/amplitude-js/amplitude.umd.js:5:2)
    at Module._compile (node:internal/modules/cjs/loader:1095:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1124:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:816:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at require (node:internal/modules/cjs/helpers:93:18)
    at Object.411 (/Users/kimdonghyeon/repos/EveryAnalytics/react-analytics-provider/demo/with-nextjs/.next/server/pages/_app.js:150:18) {
  type: 'ReferenceError'

Related Issues

related #209

Checklist ✋

PR 타이틀을 {PR type}: {PR title}로 맞췄습니다. (type 예시: feat | fix | BREAKING CHANGE | chore | ci | docs | style | refactor | perf | test) (참고: conventional commits)
모든 변경점들을 확인했으며 적절히 설명했습니다.
빌드와 테스트가 정상적으로 수행됨을 확인했습니다. (yarn build, yarn test)
깃헙 이슈를 연결하겠습니다. (커밋에 resolve #이슈넘버 적거나 PR생성 후 Linked Issue 추가)

codecov-commenter · 2021-09-13T13:03:15Z

Codecov Report

Merging #211 (03faef8) into main (aa331cb) will decrease coverage by 0.77%.
The diff coverage is 46.15%.

@@            Coverage Diff             @@
##             main     #211      +/-   ##
==========================================
- Coverage   89.47%   88.69%   -0.78%     
==========================================
  Files          19       19              
  Lines         114      115       +1     
  Branches        8        8              
==========================================
  Hits          102      102              
- Misses         12       13       +1

Impacted Files	Coverage Δ
src/contexts/AnalyticsProviderContext.ts	`36.36% <22.22%> (-3.64%)`	⬇️
src/contexts/useAnalytics.ts	`75.00% <100.00%> (ø)`
src/hooks/useAnalyticsPageView.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa331cb...03faef8. Read the comment docs.

greatSumini · 2021-09-14T13:08:27Z

좋은 의견 감사합니다!
현재의 RAP는 정말 Provider 역할만 하고 있는데, 이런 방향으로 가면 리액트 기반 웹 분석 프레임워크라고 불러도 될 것 같네요 ㅎㅎ
전 재밌고 유용할 것 같아서 좋습니다! 다만 고민과 작업이 많이 필요하긴할 것 같네요 ㅎㅎ

wplong11 · 2021-09-16T02:35:43Z

@greatSumini 고민과 작업이 많이 필요한 부분은 모듈 분리 작업 쪽을 말씀하시는 것이겠죠?

기존에 구현되어있던 코드를 조금 수정해서 다음과 같이 기능별 경계를 조금 더 뚜렸하게 나눈게 전부고, 앞으로 구현해야 하는 코드 양은 차이가 없는 것 같아요

Product Analytics를 쉽게 사용할 수 있는 공용 인터페이스
- 원래 정의했던 것인데, 리액트에서 사용하는 개념(Context)만 제거함
PA 별 인터페이스 구현체
- 원래 구현하던 것인데, 공용 PA 인터페이스를 따라 구현하도록 방향만 바뀜. 우리가 새로운 PA를 지원하기 위해 구현체를 추가할 때 필요한 작업량은 동일함. 대신 기존에 작성된 구현체가 공용 PA 인터페이스를 따르도록 수정하는 작업은 필요. 큰 작업은 아님
PA 공용 인터페이스를 리액트에서 쉽게 쓸 수 있게하는 도구
- 기존 RAP가 제공하려던 것. 이 PR 이후로 손 댈 부분 없음

milooy

PA 별 지원 모듈은 Analytics 인터페이스의 구현체를 제공하면 됩니다.
class GoogleAnalytics implements Analytics { /* ... / }
class AmplitudeAnalytics implements Analytics { / ... / }
class FirebaseAnalytics implements Analytics { / ... / }
class AppsFlyerAnalytics implements Analytics { / ... */ }

이 부분이 잘 이해가 가지 않는데,
PA별로 Analytics(initialize, trackEvent 등) 를 따로 implement하는게 그림이 그려지지 않네요

Analytics 와 PA구현체는 의존성 제로이지 않나요? 👀

milooy · 2021-09-26T08:03:31Z

demo/with-cra/src/components/NavBar.tsx


 const NavBar = () => {
-  const analytics = useAnalyticsContext();
+  const analytics = useAnalytics();


Q. Context라는 표현이 불필요해 보여서 지웠습니다. 리액트에서 많이 쓰는 관례인가요? 저는 리액트를 잘 모릅니다 🙏

A. 관례긴 한데 요즘은 생략도 많이 합니다. 우리도 생략해요~ 👍

milooy · 2021-09-26T08:20:25Z

demo/with-cra/src/components/NavBar.tsx

        href="/"
        onClick={() => {
-          analytics.onEvent('Click logo');
+          analytics.trackEvent('Click logo');


아예
analytics.event('click logo')
analytics.click()
analytics.set() // <- 여긴 이미 해주심

으로 짧게가면 어떨까요? ㅎㅎ 굿
기존에 on은 필요 없는게 맞겠어용

set에 track이 안 붙은 이유는 아래 스레드에 있어요

track이 붙은 건 이벤트 추적에(기록) 사용되는 것이고, track이 안 붙은 경우는 이벤트 추적에 사용되는 메서드가 아니에요!

#211 (comment)

milooy · 2021-09-26T08:23:57Z

README.md

+const analytics: Analytics = {
+  initialize: () => console.log('onInitialize'),
+  trackPageView: params => console.log('onTrackPageView', params),
+  trackEvent: (name, params) => console.log('onTrackEvent', name, params),
+  trackClick: (name, params) => console.log('onTrackClick', name, params),
+  set: (...args) => console.log('onSet', args),
+  setUserId: userId => console.log("onSetUserId", userId),
+  setUserProperty: params => console.log("onSetUserProperty", params),
+}

 <AnalyticsProvider
  onInitialize={() => console.log('initialized')}
-  onPageView={(params) => console.log('pageview', params)}
-  onEvent={(name, params) => console.log('event', name, params)}
-  onClick={(name, params) => console.log('click', name, params)}
-  onSetUserId={(userId) => console.log('setUserId', userId)}
+  analytics={analytics}
 >


track이 붙은곳과 아닌곳이 혼재되어있어 헷갈릴거같아요
동사(set)로 시작하는곳엔 track을 생략해주신거같은데
'set을 추적한다' 니까 trackSet이 더 맞지 않을까요?

track- 과 on- 중에 어떤게 더 직관적인지 고민고민..

track: 트래킹한다가 명확히 보임

on: 이벤트리스너임이 명확히 보임

track이 붙은곳과 아닌곳이 혼재되어있어 헷갈릴거같아요
동사(set)로 시작하는곳엔 track을 생략해주신거같은데
'set을 추적한다' 니까 trackSet이 더 맞지 않을까요?

track이 붙은 건 이벤트 추적에(기록) 사용되는 것이고, track이 안 붙은 경우는 이벤트 추적에 사용되는 메서드가 아니에요! set은 이벤트 기록이(tracking) 아니라 UserProperty 같은 걸 저장할 때 쓰는 거라고 이해했기 때문에 trackSet이 아니라 set이라고 썼어요

react-analytics-provider/src/utils/googleAnalytics/set.ts

Line 9 in d381609

/** Allows you to set values that persist across all the subsequent gtag() calls on the page.

track- 과 on- 중에 어떤게 더 직관적인지 고민고민..
track: 트래킹한다가 명확히 보임
on: 이벤트리스너임이 명확히 보임

저는 Analytics를 호출하는 곳을 고려해서 track을 선택했어요. Analytics를 사용하는 곳에서 이벤트 리스너를 호출한다는 개념보다 Analytics에 이벤트 트래킹해달라고 명령하는 개념으로 보는 것이 자연스럽다고 느껴졌어요. 이벤트 리스너를 호출한 결과로 이벤트가 트래킹된다는 것 보다 의도가 직접적으로 표현되기도 하고요!

stale · 2021-11-25T15:46:53Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

wplong11 force-pushed the wplong11/refactor-analytcis branch 7 times, most recently from 870e008 to 91edbd5 Compare September 13, 2021 13:01

wplong11 force-pushed the wplong11/refactor-analytcis branch from 91edbd5 to f304390 Compare September 13, 2021 13:04

wplong11 added 6 commits September 13, 2021 22:06

refactor: rename the AnalyticsProviderContext to Analytics

8702d43

refactor: rename useAnalyticsContext to useAnalytics

edb6bc7

refactor: extract Analytics to a separate file

cec9c56

refactor: rename method name of Analytics

36dc600

refactor: modify props structure of AnalyticsProvider

de8326f

feat: add initialize method to Analytics

03faef8

wplong11 force-pushed the wplong11/refactor-analytcis branch from f304390 to 03faef8 Compare September 13, 2021 13:06

milooy reviewed Sep 26, 2021

View reviewed changes

stale bot added the stale automatically marked as stale because it has not had recent activity. label Nov 25, 2021

stale bot closed this Dec 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: change RAP interface design #211

refactor: change RAP interface design #211

Uh oh!

wplong11 commented Sep 12, 2021 •

edited

Loading

Uh oh!

codecov-commenter commented Sep 13, 2021 •

edited

Loading

Uh oh!

greatSumini commented Sep 14, 2021 •

edited

Loading

Uh oh!

wplong11 commented Sep 16, 2021 •

edited

Loading

Uh oh!

milooy left a comment

Uh oh!

milooy Sep 26, 2021

Uh oh!

milooy Sep 26, 2021 •

edited

Loading

Uh oh!

wplong11 Sep 26, 2021 •

edited

Loading

Uh oh!

milooy Sep 26, 2021

Uh oh!

wplong11 Sep 26, 2021 •

edited

Loading

Uh oh!

stale bot commented Nov 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

refactor: change RAP interface design #211

refactor: change RAP interface design #211

Uh oh!

Conversation

wplong11 commented Sep 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

작업 내용

Help Wanted 👀

Related Issues

Checklist ✋

Uh oh!

codecov-commenter commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

greatSumini commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wplong11 commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milooy left a comment

Choose a reason for hiding this comment

Uh oh!

milooy Sep 26, 2021

Choose a reason for hiding this comment

Uh oh!

milooy Sep 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wplong11 Sep 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milooy Sep 26, 2021

Choose a reason for hiding this comment

Uh oh!

wplong11 Sep 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Nov 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wplong11 commented Sep 12, 2021 •

edited

Loading

codecov-commenter commented Sep 13, 2021 •

edited

Loading

greatSumini commented Sep 14, 2021 •

edited

Loading

wplong11 commented Sep 16, 2021 •

edited

Loading

milooy Sep 26, 2021 •

edited

Loading

wplong11 Sep 26, 2021 •

edited

Loading

wplong11 Sep 26, 2021 •

edited

Loading