-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: Refactor set function and implement setUserProperty function with demo #113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
마이너라 어프룹합니다ㅏ~
이 방향 좋아용 ㅎㅎ
Co-authored-by: Yurim Jalynne Jin <jayjinjay@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
룩굿투미!
미리 정의된 사용자 속성과 커스텀 사용자 속성 차이군요 :) |
Description
제가 맡은 부분은 setUserProperty인데 문서 에 보니 이런 형태더라구요.
일반적인(?) set은 아래처럼 params만 넘겨주는 형태인 것 같은데 user property나 advertise feature를 disable한다든지 몇몇 경우에 한해서 Name이 필요한 듯합니다.
그래서 일단 기존에 params만 받도록 되어 있던 set 함수를 오버로딩해서 name도 받을 수 있게 추가했습니다. 여기선 name이 옵셔널이고 params가 필수라 오버로딩하지 않고 하려면
set(params: UnknownRecord, name?: string)
이렇게 해야하는데 다른 함수들은 전부 그 반대라 사용할 때 헷갈릴 것 같았거든요Update 8.29
저번 커밋에서는 오버로딩을 사용해서 했었는데 실제 데모에서 적용하려고 보니까 오버로딩 함수는 parameter type을 뽑아내기가 어려운 문제가 있더라구요. Provider에서 넣어주는 onSet의 아규먼트를 googleAnalytics, fruitLogger 등의 함수에 rest parameter(...args)로 그대로 넘겨주려고 했는데 오버로딩된 타입을 인식을 제대로 못해서 에러가 납니다. (설명이...어렵네요..)
ts에 관련 이슈가 있는데 아직 오픈 상태이고, 스택오버플로우에서 비슷한 문제 상황을 찾아서 tuple union 방식으로 다시 바꿨습니다. 만약 저희 타입스크립트 버전이 4.2 이상이었다면 이런 문법으로도 가능했을 것 같네요.
Help Wanted 👀
요거 괜찮으면 set사용해서 setUserProperty라는 함수 따로 또 만들고 Demo에도 예시 추가하는 작업하려고 합니다!피드백 부탁드립니다~
Related Issues
resolve #70
resolve #162
Checklist ✋
npm run build
,npm run test
)