analytics-next
analytics-next copied to clipboard
feat: change api to be functional
Hey there, I noticed that the classes introduced define all the functions as static. Such a thing is a sign of code smell since it is leveraging the class (object) as a bag of functionalities which you can always achieve with import * as AnalyticsBrowser in ESM.
Removing such class definition and exposing the functions will give a cleaner, simple, and most important minifying code that will be shorter since you don't need the full class name.
Downside
Exposing everything from the root is suboptimal (reasons why I had to rename the functions), ideally, you would add two more exposed modules or as many runtime environments. For example:
import * as AnalyticsBrowser from '@segment/analytics-next/browser'
import * as AnalyticsNode from '@segment/analytics-next/node'
Follow up
I can fix the exporting, just wanna showcase what I meant rather than go back and forward on it.
Also fix the integration side, since such a file would probably create an object that mimics the previous behavior. Friendly when people don't import the script as part of their app.
Hey peeps @chrisradek @pooyaj 👋🏻 do you mind helping me with this one?
Just want to know if you are interested at all in the topic of bundle size at the very least.
@yordis Definitely interested in the topic of bundle sizes. The thing we need to be very careful with changes like these are making sure we stay backwards-compatible with the version of the library hosted on the CDN. In theory we can make breaking changes to the npm package as long as the major version bump and the CDN version remains backwards-compatible.
I tend to agree that having classes with only static methods isn't idiomatic JS as well. Did you happen to check what the bundle size delta is with this change? We have a make size command that will check the size of the package that ends up on the CDN.
In theory we can make breaking changes to the npm package as long as the major version bump and the CDN version remains backwards-compatible.
Right, the intention is that we get to optimize the CDN version for CDN reasons, but users like me that are using the package as a dependency get to take advantage of these things in the short and long term.
Did you happen to check what the bundle size delta is with this change?
// BEFORE
Size limit: 24.7 kB
Size: 24.67 kB with all dependencies, minified and gzipped
// AFTER
Size limit: 24.7 kB
Size: 24.57 kB with all dependencies, minified and gzipped
It is not much, but also it is a lot for a simple change and sets the foundation for the future.
Also,
The reason I didn't continue improving things is that I don't know your stand on the subject matter.
I have been working closely with Sentry folks on the topic (https://github.com/getsentry/sentry-javascript/pull/4381) and I know that it will eventually matter, and I noticed that most of these classes aren't really needed but requires a buy-in to make sure it is a goal to have a maintainable code that it is also friendly for customers.
So honestly, I am taking this opportunity to learn about your direction and whatnot, and hopefully, I get to contribute and help since I think I may know how to help a bit.