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

feat: adds the use of topsort sdk for reporting events #275

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

barbmarcio
Copy link
Contributor

@barbmarcio barbmarcio commented Jul 25, 2024

This PR adds the use of Topsort.js SDK for reporting events

You can see the use inside analytics in this video

File Before (gzip) After (gzip)
dist/ts.mjs 9.04 kB (3.21 kB) 10.37 kB (3.61 kB)
dist/ts.js 6.32 kB (2.72 kB) 7.63 kB (3.22 kB)

The difference comes from:

Please feel free to check the diff here as well

@barbmarcio barbmarcio force-pushed the feat/using-topsort-sdk branch 2 times, most recently from 4abf986 to 099ec6a Compare July 25, 2024 21:05
@barbmarcio barbmarcio marked this pull request as ready for review July 25, 2024 21:05
Copy link
Contributor

@sk- sk- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Thanks for the change. My only concern is about the resulting library size and whether they builder is actually applying tree shaking for the dependencies.

Could you add to the PR description the size of the library before and after the change.

src/detector.ts Outdated Show resolved Hide resolved
src/reporter.ts Show resolved Hide resolved
@barbmarcio barbmarcio requested a review from sk- July 26, 2024 01:12
@jbergstroem
Copy link
Member

I would like to understand why the size grows. Since its so small, can you peek into the diff?

@barbmarcio
Copy link
Contributor Author

barbmarcio commented Jul 26, 2024

I would like to understand why the size grows. Since its so small, can you peek into the diff?

The difference comes from:

Please feel free to check the diff here as well

(also made it part of the description)

Copy link
Member

@jbergstroem jbergstroem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Lets wait for @sk- as well.

@barbmarcio barbmarcio merged commit 372a507 into main Jul 30, 2024
4 checks passed
@barbmarcio barbmarcio deleted the feat/using-topsort-sdk branch July 30, 2024 21:43
@barbmarcio barbmarcio changed the title feat: adding the use of topsort sdk for reporting events feat: adds the use of topsort sdk for reporting events Sep 5, 2024
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.

4 participants