-
-
Notifications
You must be signed in to change notification settings - Fork 302
chore: move analytics definitions to suite-common #19084
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
Conversation
032fcd1
to
173c269
Compare
✅ Previously successful run of [Test] PR Suite Web e2e tests workflow has been found. |
173c269
to
4eee74a
Compare
✅ Previously successful run of [Test] PR Suite Web e2e tests workflow has been found. |
4eee74a
to
018c067
Compare
✅ Previously successful run of [Test] PR Suite Web e2e tests workflow has been found. |
018c067
to
e65e86b
Compare
✅ Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
✅ Previously successful run of [Test] PR Suite Web e2e tests workflow has been found. |
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.
looks ok, but somebody from desktop and mobile should approve probably, not me.
@Lemonexe, @matejkriz maybe?
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.
Thanks, works as expected on mobile (haven't test on desktop/web). I would not take this as "the way" we will apply on more shared packages in the future, but for the analytics specifically, the solution looks good 👌
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.
LGTM. After explanation the structure is clear, so please update the README.mds and it's good to go 🚢
I tested only superficially, that some analytics in Suite Web are sent, and bear sensible values.
e65e86b
to
02bc7e6
Compare
✅ Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
✅ Previously successful run of [Test] PR Suite Web e2e tests workflow has been found. |
READMEs updated, main one is here |
QA OK Info:
|
Description
We need a solution to send analytics from Suite Common packages.
I tried several approaches but ultimately came up with this:
@suite-common/analytics
@trezor/suite-analytics
and@suite-native/analytics
will re-export those subfolders directly@suite-common/analytics
, those shared types are used and it will select the implementation based on the environment (byindex.ts
vsindex.native.ts
)🔍🖥️ Suite web test results: View in Currents
🔍🖥️ Suite desktop test results: View in Currents
🔍🖥️ Suite native android test results: View in Currents