refactor: suggest new interface #226

milooy · 2021-10-12T14:43:44Z

Description

실제 구현이 아닌 인터페이스 제안입니다~
코드리뷰로 이것저것 많은 의견 부탁드려요!
어떤점이 불편해보이는지, 어떤점이 암묵적인지 등등
질문도 환영입니다

해결하려는 문제를 다음과 같이 상호배타적인 3개 모듈로 분리합니다. (B만 A에 의존성이 있음)

A. EveryAnalytics/core

복잡한 로그 추적 코드를 일관적이고 깔끔하게 호출하고 싶을 때 사용합니다.

유저의 사전 설정에 따라 다양한 Product Analytics툴의 이벤트를
모든 환경에서 사용 가능 (React/Vue등 라이브러리 디펜던시 X)(GA, Amplitude등 PA 툴 디펜던시 X)
하나의 로깅 툴 뿐만 아니라 여러 개의 로깅 툴을 함께 쓸 때도 유용합니다.
initialize, click, pageView등 기본적인 데이터 추적 이벤트에 대한 interface를 제공합니다.

B. EveryAnalytics/react

리액트에서 더 선언적으로 로그 추적 코드를 호출하고 싶을 때 사용합니다.
EveryAnalytics/core와 함께 사용해야 합니다.

C. EveryAnalytics/helper

EveryAnalytics/helper/google-analytics
EveryAnalytics/helper/amplitude
각 PA툴에 대한 헬퍼함수를 제공합니다. 단독으로 사용 가능합니다.

Help Wanted 👀

Related Issues

resolve #
fix #

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 추가)

hiphapis · 2021-10-13T00:52:41Z

파일 체인지스에서 깃헙 액션 워닝이랑 에러뜬거 가리는 법 없나요? ㅠ,.ㅠ

milooy · 2021-10-17T07:50:21Z

파일 체인지스에서 깃헙 액션 워닝이랑 에러뜬거 가리는 법 없나요? ㅠ,.ㅠ

이거 크롬 익스텐션으로 만들어보기 좋은 주제네요 ㅋㅋ

hiphapis · 2021-10-19T00:36:33Z

demo/with-cra/src/suggestion-with-js-4.ts

+  event: (...args) => {
+    console.info('event occured! ' + args);
+  },


아래 customMethods를 통해 추가하지 않고 여기에 추가한 이유가 있을까요?

앗 여기 코드 잘못됐네요 Case2처럼 init따로 빠져있어야해요

const eventHandlers = { click: (name, params) => { logGA.click(name, params); logAmplitude.logEvent(amtid, params); }, pageView: (name, params) => { const path = window.location.pathname + window.location.search; logAmplitude.logEvent(name, { action_type: 'pageView', path, ...params }); }, customMethods: () => { return { outbound: args => { console.info('outbound occured! ' + args); fruitLogger.event('dd', { outbound: true, ...args }); }, }; }, }; Analytics.setup(initialize, eventHandlers);

근데 써놓고 생각해보니 customMethods는 세번째 인자로 넘겨도 될듯...

const eventHandlers = { click: (name, params) => { logGA.click(name, params); logAmplitude.logEvent(amtid, params); }, pageView: (name, params) => { const path = window.location.pathname + window.location.search; logAmplitude.logEvent(name, { action_type: 'pageView', path, ...params }); }, }; // generic 가능? const setUserCreatedEventHandlers = () => { return { outbound: args => { console.info('outbound occured! ' + args); fruitLogger.event('dd', { outbound: true, ...args }); }, }; }; Analytics.setup(initialize, eventHandlers, setUserCreatedEventHandlers);

두번째인자인 eventHandlers 도 사실 우리가 미리 정해준 기본 이벤트들을 custom하는 느낌이라ㅏ
세번쩨인자는 setUserCreatedEventHandlers 라고 네이밍 해봤어요

제가 제대로 이해한 것인지 모르겠는데 혹시 case2가 case3에 포함될 수 있는걸까요? 이렇게 event를 쓰는게 기본 제공 메서드인 event를 오버라이드하는 거라면 아래처럼 쓰면 case2를 커버할 수 있는 형태일까유?

Analytics.setup({ init: async () => { logGA.initialize(gaid); logAmplitude.initialize(amtid); fruitLogger.init(); }, event: (...args) => { console.info('event occured! ' + args); }, })

만약 그렇다면 case1,2,3로 나누는 것보다 디폴트 세팅만 있는 case1과 커스텀이 가능한(기본 메서드 오버라이드 가능 & customMethods도 추가 가능) case3 로만 나누어도 되지 않을까 싶어서요.

저는 기존 메쏘드 오버라이드 하는건 막았으면 좋겠어요.
기존 메쏘드는 기존 메쏘드데로, 커스텀은 커스텀데로. 구분은 namespace로

(Case3은 Case2에 customMethods만 추가한 코드예요~)

이름이 >>커스텀<<메쏘드라 불러서 개념부터 혼란이 오는거같은데

1. 기본메서드(event, click, pageview 등 무궁무진):

RAP팀이 생각하는 기본적인 분석 이벤트들.

Analytics.pageview('회원가입', {foo: 1});

호출시

Case1(심플이니셜라이즈)로 setup했을 경우: 심플이니셜라이즈에서 넣었던 GA, Amplitude의 pageview콜이 불린다

Case2(커스텀이니셜라이즈)로 setup했을 경우: 커스텀이니셜라이즈에서 넣었던 pageview함수(기본메서드)가 호출된다.

2. 커스텀메서드(a.k.a. UserCreatedEventHandlers):

기본메서드에서 제공해주지 않는 유니크한 메서드를 추가하고 싶을 때

여기서 다영, 요한님은 init, event만 기본메서드, 나머지는 모두 customMethod로 유저 필요에 따라 추가해줘야하는 메서드라고 설계하시고,
저는 기본메서드에 init, event뿐만아니라 pageview, gaItemView 등 여러 함수들을 넣어둔다고 설계했다는 차이가 있을거같아요
웬만한 상황에서는 type추론이 되는 기본메서드들을 쓰면 좋을거같아서요!
그리고 심플이니셜라이즈 생각하면 GA, Amplitude에서 제공하는 대부분의 함수들을 기본메서드에 넣어줘야하니 기본메서드가 많아지는건 무방할거같기도 한..?

어떻게 생각하셔요?

쪼금 헷갈리는 부분이 있었는데 자세히 써주셔서 넘 감사합니다🙏 저도 rap에서 심플 이니셜라이즈를 제공하는 목적을 생각해보면 사람들이 이 라이브러리를 써서 편리할 수 있도록 하는 것이기 때문에 저희가 제공하는 기본 메서드들이 많은건 무방할 것 같습니다! 저희가 제공하는 기본 메서드 리스트를 보고 이런 것도 트래킹할 수 있구나/하면 좋겠구나 생각할 수도 있을 것 같아요!

hiphapis · 2021-10-19T00:41:53Z

demo/with-cra/src/suggestion-with-js-4.ts

+  },
+  customMethods: () => {
+    return {
+      outbound: args => {


customMethods 안에 감싸져 있는 함수의 args는 유저가 보낸 파라미터 그대로 전달하는걸까요?
아니면 Analytics에서 전처리를 한걸 넘기는걸까요?

유저가 보낸 파라미터 그대로 전달해도 무방할 것 같긴하네요.

유저 그대로요!

hiphapis · 2021-10-19T00:42:53Z

demo/with-cra/src/suggestion-with-js-4.ts

+Analytics.setup({
+  googleAnalytics: {trackingId: GA_ID, persistentValues: {id: 123}},
+  amplitude: {trackingId: GA_ID},
+});


오!! setup 아주 좋아요!!

깔끔하네요 ㅎㅎ 👍🏻 👍🏻

alledy · 2021-10-19T12:27:00Z

demo/with-cra/src/suggestion-with-js-4.ts

+    logGA.click(name, params);
+    logAmplitude.logEvent(amtid, params);
+  },
+  pageView: (name, params) => {


요기는 event인데 pageView로 혹시 잘못 쓰신걸까요 아니면 기본 제공 메서드에 pageView도 들어가는걸까요?
제가 이해한 내용은 아래와 같습니다!

Case1이 default 세팅. click, event와 같은 기본 메서드가 제공됨.

Case2는 커스텀 init과 기본 제공 메서드인 click, event를 유저가 커스텀하여 오버라이드 가능.

Case3는 customMethods라는 필드를 통해 커스텀 메서드를 유저가 직접 추가 가능.

제가 이해한거는
click은 overwrite한것이고
pageView는 커스텀 메쏘드 입니다. Case3의 customMethods에 해당하는거죠

기본제공 메서드는 저희가 무궁무진하게 넘길수있을거같아요
click, event, pageview, impression 등등

그 외에 커스텀이 필요하다면 커스텀메서드로 추가하면 되지 않을까요? 웬만해선 쓸 일 없으거라 기대하긴 해용

alledy · 2021-10-19T13:06:25Z

demo/with-cra/src/suggestion-with-js-4.ts

+  event: (...args) => {
+    console.info('event occured! ' + args);
+  },


제가 제대로 이해한 것인지 모르겠는데 혹시 case2가 case3에 포함될 수 있는걸까요? 이렇게 event를 쓰는게 기본 제공 메서드인 event를 오버라이드하는 거라면 아래처럼 쓰면 case2를 커버할 수 있는 형태일까유?

Analytics.setup({ init: async () => { logGA.initialize(gaid); logAmplitude.initialize(amtid); fruitLogger.init(); }, event: (...args) => { console.info('event occured! ' + args); }, })

만약 그렇다면 case1,2,3로 나누는 것보다 디폴트 세팅만 있는 case1과 커스텀이 가능한(기본 메서드 오버라이드 가능 & customMethods도 추가 가능) case3 로만 나누어도 되지 않을까 싶어서요.

greatSumini · 2021-10-19T15:59:23Z

demo/with-cra/src/suggestion-with-js-4.ts

+Analytics.setup({
+  googleAnalytics: {trackingId: GA_ID, persistentValues: {id: 123}},
+  amplitude: {trackingId: GA_ID},
+});


깔끔하네요 ㅎㅎ 👍🏻 👍🏻

greatSumini · 2021-10-19T16:05:47Z

demo/with-cra/src/suggestion-with-js-4.ts

+  event: (...args) => {
+    console.info('event occured! ' + args);
+  },
+  customMethods: () => {


customMethods가 이번 구현 스펙 내에 포함되는지 궁금합니다!

각 PA별 trackingId등 최소한의 파라미터만 제공해서 사용할 수 있다.

제공된 Analytics 인터페이스에 맞게 함수 구현체를 입력 받아 singleton으로 사용할 수 있다.

제공된 Analytics 인터페이스에서 벗어난 함수를 마음대로 추가해서 사용할 수 있다.

저번 회의에서 1, 2, 3중 3은 다음 마일스톤에서 대응하고, 일단 1차 릴리즈에선 기본적인 기능들로만 구성하는 것으로 이해했어서요 ㅎㅎ
혹싀 이후 논의에서 변경된걸까요 ?.?

맞아요. 그렇다면 유림님이 제안하신 PR에서 Case1만 해당되는건가요?

전체 그림 보기 위해 넣어봤어요! 3은 MVP에서 빼는걸로 해요 ㅎㅎ

넵 Case1처럼 간단하게 설정하는 기능과, Analytics 인터페이스의 구현체를 넘겨받는 preset 기능까지 두가지로 이해하고 있었습니다!

milooy · 2021-10-17T12:58:29Z

demo/with-cra/src/suggestion-with-js-3.ts

+
+button.addEventListener('click', () => {
+  Analytics.event('회원가입', {name: 'RAP'});
+  Analytics.custom.outbound('아웃바운드'); // 이렇게하면 타입추론이 좀 더 괜찮으려나?


다영: 타입추론 문제가 아니라면 그냥 Analytics.outbound해도 되지않을까? 근데 TS서폿이 이렇게 해서만 된다면

요한: 커스텀에 네임스페이스 추가하는건 괜찮다. 내가 만든거랑 라이브러리 제공이랑 명확히 구분된다면 가능

milooy · 2021-10-17T13:01:05Z

demo/with-cra/src/suggestion-with-js-3.ts

+  event: (...args) => {
+    console.info('event occured! ' + args);
+  },
+  customTrackers: (): {outbound(args: {name: string}): void} => {


수민: generic으로 customTrakers를 추적하려면
preset자체에 generic을 주던가 Analytics를 export해서 사용해야 할 수 이따!

milooy · 2021-10-17T13:02:59Z

demo/with-cra/src/suggestion-with-js-3.ts

+button.addEventListener('click', () => {
+  Analytics.event('회원가입', {name: 'RAP'});
+  Analytics.custom.outbound('아웃바운드'); // 이렇게하면 타입추론이 좀 더 괜찮으려나?
+});


수민: 린분석 과몰입

사람들은 헬퍼를 주면 좋아할거야

헬퍼 싫어 나는 PA별로 필요한 파라미터만 넣을거야 (defaultAnalytics)

파라미터에 커스텀 펑션까지 넣으면 사람들은 좋아할거야

여기서 우리가 일단 집중해야할거엔 3번은 아니라고 생각한다~

milooy · 2021-10-19T10:07:59Z

demo/with-cra/src/suggestion-with-js-4.ts

+  },
+  customMethods: () => {
+    return {
+      outbound: args => {


유저 그대로요!

milooy · 2021-10-19T10:15:56Z

demo/with-cra/src/suggestion-with-js-4.ts

+  event: (...args) => {
+    console.info('event occured! ' + args);
+  },


앗 여기 코드 잘못됐네요 Case2처럼 init따로 빠져있어야해요

const eventHandlers = { click: (name, params) => { logGA.click(name, params); logAmplitude.logEvent(amtid, params); }, pageView: (name, params) => { const path = window.location.pathname + window.location.search; logAmplitude.logEvent(name, { action_type: 'pageView', path, ...params }); }, customMethods: () => { return { outbound: args => { console.info('outbound occured! ' + args); fruitLogger.event('dd', { outbound: true, ...args }); }, }; }, }; Analytics.setup(initialize, eventHandlers);

근데 써놓고 생각해보니 customMethods는 세번째 인자로 넘겨도 될듯...

const eventHandlers = { click: (name, params) => { logGA.click(name, params); logAmplitude.logEvent(amtid, params); }, pageView: (name, params) => { const path = window.location.pathname + window.location.search; logAmplitude.logEvent(name, { action_type: 'pageView', path, ...params }); }, }; // generic 가능? const setUserCreatedEventHandlers = () => { return { outbound: args => { console.info('outbound occured! ' + args); fruitLogger.event('dd', { outbound: true, ...args }); }, }; }; Analytics.setup(initialize, eventHandlers, setUserCreatedEventHandlers);

두번째인자인 eventHandlers 도 사실 우리가 미리 정해준 기본 이벤트들을 custom하는 느낌이라ㅏ
세번쩨인자는 setUserCreatedEventHandlers 라고 네이밍 해봤어요

milooy · 2021-10-20T02:59:56Z

demo/with-cra/src/suggestion-with-js-4.ts

+    logGA.click(name, params);
+    logAmplitude.logEvent(amtid, params);
+  },
+  pageView: (name, params) => {


기본제공 메서드는 저희가 무궁무진하게 넘길수있을거같아요
click, event, pageview, impression 등등

그 외에 커스텀이 필요하다면 커스텀메서드로 추가하면 되지 않을까요? 웬만해선 쓸 일 없으거라 기대하긴 해용

milooy · 2021-10-20T03:01:17Z

demo/with-cra/src/suggestion-with-js-4.ts

+  event: (...args) => {
+    console.info('event occured! ' + args);
+  },
+  customMethods: () => {


전체 그림 보기 위해 넣어봤어요! 3은 MVP에서 빼는걸로 해요 ㅎㅎ

milooy · 2021-10-20T03:16:55Z

demo/with-cra/src/suggestion-with-js-4.ts

+  event: (...args) => {
+    console.info('event occured! ' + args);
+  },


(Case3은 Case2에 customMethods만 추가한 코드예요~)

이름이 >>커스텀<<메쏘드라 불러서 개념부터 혼란이 오는거같은데

1. 기본메서드(event, click, pageview 등 무궁무진):

RAP팀이 생각하는 기본적인 분석 이벤트들.

Analytics.pageview('회원가입', {foo: 1});

호출시

Case1(심플이니셜라이즈)로 setup했을 경우: 심플이니셜라이즈에서 넣었던 GA, Amplitude의 pageview콜이 불린다

Case2(커스텀이니셜라이즈)로 setup했을 경우: 커스텀이니셜라이즈에서 넣었던 pageview함수(기본메서드)가 호출된다.

2. 커스텀메서드(a.k.a. UserCreatedEventHandlers):

기본메서드에서 제공해주지 않는 유니크한 메서드를 추가하고 싶을 때

여기서 다영, 요한님은 init, event만 기본메서드, 나머지는 모두 customMethod로 유저 필요에 따라 추가해줘야하는 메서드라고 설계하시고,
저는 기본메서드에 init, event뿐만아니라 pageview, gaItemView 등 여러 함수들을 넣어둔다고 설계했다는 차이가 있을거같아요
웬만한 상황에서는 type추론이 되는 기본메서드들을 쓰면 좋을거같아서요!
그리고 심플이니셜라이즈 생각하면 GA, Amplitude에서 제공하는 대부분의 함수들을 기본메서드에 넣어줘야하니 기본메서드가 많아지는건 무방할거같기도 한..?

어떻게 생각하셔요?

hiphapis · 2021-10-21T09:01:51Z

@milooy target branch를 main말고 다른 브랜치로 하면 어떨까요? Changes 볼려고 하면 브라우져가 힘들어해서.. ㅠㅠ

stale · 2021-12-20T09:57:15Z

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.

jhlee910609 and others added 14 commits September 30, 2021 09:38

chore:remove local dependencies and set yarn berry

bea5801

chore:using yarn workspace

0902f02

chore:fix RAP version

fed8d63

Match TS version

d3269ed

fix relative .yarn path in demo/with-cra

0959398

Remove unncessesory README

bbd3013

call amplitude instance SSR safely and add examples in next app

6ea3818

Update CI - npm to yarn

1d80664

add dependencies related to linting

e4493a1

Fix test

95d3afa

Remove ignoring /demo in eslint to lint check entire app

2b6a77e

Fix test: add async/await when calling amplitude utils

8648022

0.3.0

926a40e

Add demos

45896e0

Base automatically changed from feat/216 to main October 15, 2021 00:44

defaultAnalytics 유즈케이스 추가

36d6417

Add 4th option

47cc477

hiphapis reviewed Oct 19, 2021

View reviewed changes

alledy reviewed Oct 19, 2021

View reviewed changes

greatSumini reviewed Oct 19, 2021

View reviewed changes

milooy commented Oct 20, 2021

View reviewed changes

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

stale bot closed this Dec 27, 2021

refactor: suggest new interface #226

refactor: suggest new interface #226

Uh oh!

Conversation

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

Uh oh!

Description

A. EveryAnalytics/core

B. EveryAnalytics/react

C. EveryAnalytics/helper

Help Wanted 👀

Related Issues

Checklist ✋

Uh oh!

hiphapis commented Oct 13, 2021

Uh oh!

milooy commented Oct 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

1. 기본메서드(event, click, pageview 등 무궁무진):

2. 커스텀메서드(a.k.a. UserCreatedEventHandlers):

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

1. 기본메서드(event, click, pageview 등 무궁무진):

2. 커스텀메서드(a.k.a. UserCreatedEventHandlers):

Uh oh!

hiphapis commented Oct 21, 2021

Uh oh!

stale bot commented Dec 20, 2021

Uh oh!

milooy commented Oct 12, 2021 •

edited

Loading