Skip to content

Conversation

@yordis
Copy link
Contributor

@yordis yordis commented Mar 11, 2022

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.

@yordis yordis force-pushed the global-object-impr branch 2 times, most recently from bad1fd7 to 0733569 Compare March 11, 2022 01:14
@yordis yordis force-pushed the global-object-impr branch from 0733569 to c58b978 Compare March 11, 2022 01:18
@yordis
Copy link
Contributor Author

yordis commented Mar 31, 2022

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.

@chrisradek
Copy link
Contributor

@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.

@yordis
Copy link
Contributor Author

yordis commented Mar 31, 2022

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 (getsentry/sentry-javascript#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.

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.

2 participants