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

Avoid using @babel/runtime as a dependency #421

Closed
theoludwig opened this issue May 25, 2024 · 8 comments · Fixed by #431
Closed

Avoid using @babel/runtime as a dependency #421

theoludwig opened this issue May 25, 2024 · 8 comments · Fixed by #431
Labels
context-v2 Related to tailwind-merge v2

Comments

@theoludwig
Copy link

Describe the bug

While it is expected that tailwind-merge increase bundle size (as said in the docs) and has a cost at runtime, we should minimze it nonetheless.

We should be able to use modern JavaScript syntax and features, as supported by modern browsers, hence, I don't think adding @babel/runtime (for better compatibility? as I understand it) is worth it.

To Reproduce

npm install tailwind-merge

Expected behavior

No dependencies.

Environment

tailwind-merge@2.3.0

@dcastil dcastil added context-v2 Related to tailwind-merge v2 labels May 29, 2024
@dcastil
Copy link
Owner

dcastil commented May 29, 2024

Hey @theoludwig! 👋

tailwind-merge is bundled to be compatible with the browserslist query > 0.5%, last 2 versions, Firefox ESR, not dead, maintained node versions (code) which is the default for most apps.

Do you have a use case where speed matters so much more than compatibility with enough browsers? The tradeoff with not transpiling at all to shave off a few bytes is that the code will break the app for some 10-20% of users.

@theoludwig
Copy link
Author

Hey @dcastil, thanks for your reply!

I understand the desire to maintain broad compatibility, but I would like to present a case for why we should avoid including the @babel/runtime dependency in the tailwind-merge library.

The tradeoff with not transpiling at all to shave off a few bytes is that the code will break the app for some 10-20% of users.

Maybe, but then, for 80-90% of users, they are downloading and executing more JavaScript, than necessary.
Also, I'm not convinced, that removing @babel/runtime, it ill break for 10-20% users, it really depends of the JavaScript features used by tailwind-merge.

As you said: "bundled to be compatible with the browserslist query", "which is the default for most apps.", that's exactly the point, it's not the library developer responsibility, but the app developer responsibility to bundle/polyfill/transpile accordingly to what browsers they want to target. Most frameworks for apps, already handle it, whether it is Vite.js, Next.js or others.

For reference, some comments/articles, that discuss this:

Conclusion

In conclusion, while ensuring compatibility is important, it should not come at the cost of performance and maintainability for the majority of users. By not including @babel/runtime and allowing application developers to handle polyfills, we can create a more efficient and maintainable library that performs optimally in modern environments.

Looking forward to your thoughts on this!

@dcastil
Copy link
Owner

dcastil commented May 30, 2024

That argument makes a lot of sense to me, thanks for bringing it up!

I won't be able to stop transpiling the default bundle because that could break a lot of apps. But I could add another bundle, e.g. esnext that is only converted to JS without adding any polyfills. You would then need to use it like this in your code:

import { twMerge } from 'tailwind-merge/esnext'

There is already a similar bundle like that for people that need the code to be ES5-compatible (#286, #344):

import { twMerge } from 'tailwind-merge/es5'

Would that be something that could work for you?

@theoludwig
Copy link
Author

Yes, possibly, a 'tailwind-merge/esnext'. 😄
And the dependency @babel/runtime can't be removed?

@dcastil
Copy link
Owner

dcastil commented May 30, 2024

That bundle wouldn't use @babel/runtime then. That's what I expect. 🤔 So when you import tailwind-merge/esnext, the Babel runtime wouldn't be in your import tree.

@dcastil
Copy link
Owner

dcastil commented Jun 21, 2024

@theoludwig just checked the bundle and the normal tailwind-merge bundle doesn't even import @babel/runtime. You'll find the ES module in node_modules/tailwind-merge/dist/bundle-mjs.mjs and can see that there are no imports. It's only imported in the ES5 bundle in node_modules/tailwind-merge/dist/es5/bundle-mjs.mjs.

@dcastil
Copy link
Owner

dcastil commented Jun 21, 2024

But I can see how having @babel/runtime as a dependency of tailwind-merge just for the ES5 bundle is annoying. I'll remove the dependency and inline the transform functions into the ES5 bundle since that isn't optimized for code size anyway.

Copy link

github-actions bot commented Jul 7, 2024

This was addressed in release v2.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context-v2 Related to tailwind-merge v2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants