[styles] Replace classnames with clsx

[TrySound]

clsx new faster replacement for
classnames with esm support.

• I have followed (at least) the PR section of the contributing guide.

[mbrookes]

Benchmark https://github.com/lukeed/clsx/tree/master/bench, although this is in isolation, so the proportional benefit to component mount time may be negligible...

[TrySound]

The main benefit for me is esm support which allows me to skip cjs-to-esm conversion for my rollup build.
One more nice thing is that clsx is shorter than classNames.

[TrySound]

Original package is umd which is useless for bundling.
https://unpkg.com/classnames@2.2.6/index.js

[oliviertassinari]

One more nice thing is that clsx is shorter than classNames.

This is something I have been wondering about. What do you guys think of this change?

-import classNames from 'classnames';
+import cx from 'classnames';

[TrySound]

I will prefer clsx now. We may even mention about it in docs. People will use whatever appears in this project. I will migrate pickers too.

[oliviertassinari]

The release of lukeed/clsx@08d028f is a blocker.

[eps1lon]

Please open a separate issue/PR for the classNames -> cx rename. I'm curious about arguments for name mangling in dev code.

As for the classnames vs clsx argument:
What do you achieve by having esm support over cjs support? I only ever thought about this in terms of tree shaking and since both are just single function packages I don't see where you would gain bytes by tree shaking.

Is a micro benchmark really the argument here? Does anybody have stats on portion of execution time a classNames call has over a render call?

Note about bundle size: Again this only apply in isolation. How many packages are already pulling in classnames into their bundle? If they do then this will create a 200B overhead. If not then how many bytes would this save over classnames?

We should have answers to this before throwing out a battle tested package with 2.4M downloads/week over a 18 days old package that has 150 downloads/week. Even if the author is well known we should not make these decisions lightly.

[oliviertassinari]

I have opended JedWatson/classnames#187.

[TrySound]

It was already asked. They afraid legacy.
JedWatson/classnames#150 (comment)

Treeshaking will work with clsx. ClassNames is not treeshakable because of umd wrapper. So makes sense.

@oliviertassinari People will use what we recommend. Changing package in favour of more maintainable is not a problem. I will propose tiny-warning soon because its maintainer didn't stuck on mirorring facebook once in a year without a chance for improvement from the community.

[eps1lon]

Treeshaking will work with clsx. ClassNames is not treeshakable because of umd wrapper. So makes sense.

What do you shake from a one function package is what I'm asking.

[TrySound]

@eps1lon If classnames is a transitive dependency and material-ui being imported from index.js (assume it's fully treeshakable) but imported component does not use classnames then it still will be appeared in the bundle. clsx will not.

[lukeed]

Hey there, clsx author here 👋

Thanks for the consideration, very cool! I'm not trying to advocate clsx here – to be honest, I don't really care if it gets used or not – I think the community's benefit should always be first priority, given the success of this project.

That said, I just want to address a few points:

• clsx is actually the successor of obj-str, which is 2 years old. Though still not nearly as popular, this code has been in production for my clients/projects for 2 years. It's released as a new name to preserve obj-str & its purposes.

• The difference between 200B & 450B (whatever it is) is essentially negligible, yes.

• The benchmarks belong to classnames & are what they rely on heavily before accepting any PR. I did not expect/know clsx was faster until I stumbled upon them while looking for compatibility tests.

• Code duplication is certainly a concern, and IMO, the only concern to really consider. I'm not familiar with the full mui ecosystem, but if there are popular boilerplates/kits that have been co-installing classnames for years, then shifting will be problematic – at least for a period. This can be amended with package aliases & whatnot, but still requires a shift.

• While both export a single function, a CommonJS module isn't treated the same in Webpack/Rollup as an ESM file. There's overhead (see rollup-plugin-commonjs) in parsing the file to read/normalize what the exports may be. While you can solve this with config, it requires as much/more work to fix within a community than it is to alias/swap packages.

Anyways, that's it for now~! Will check back in a few days/weeks, but I'm happy no matter the outcome.

Thanks again!

[eps1lon]

@eps1lon If classnames is a transitive dependency and material-ui being imported from index.js (assume it's fully treeshakable) but imported component does not use classnames then it still will be appeared in the bundle. clsx will not.

I could've sworn that this is not the case so I tried this with webpack@4.22.0 and import { colors } from '@material-ui/core' which does not use classnames and this will not include classnames in the bundle. Could you show me an example where a module is shaken from the bundle but its transitive dependencies are still included?

[TrySound]

Rollup will include it

[eps1lon]

Rollup will include it

Could you provide some context why rollup does not solve this issue when webpack is capable of solving this without additional configuration? Sounds like a fix to rollup would be more appropriate
since this would imply that we should make sure that every dependency we have is supporting esm support.

[TrySound]

I'd say the problem is sideEffects. It gives webpack uncontrollable ability to remove whole modules if they are not imported. Even if they have side effects. Rollup only analyzes code. classnames code does not fit in current reality because of UMD wrapper.

[eps1lon]

Rollup only analyzes code. classnames code does not fit in current reality because of UMD wrapper.

I was very skeptical about this statement. If rollup would only analyze code then it shouldn't have a reason to include classnames because it should never reach code that requires classnames. So I went ahead and created a minimal repro: https://github.com/eps1lon/rollup-shaken

Using import colors from "@material-ui/core/colors"; will correctly exclude the classnames package from the bundle. However using import { colors } from "@material-ui/core"; will include it. But this is due to the fact that rollup will include all components from @material-ui/core which do require classnames.

This indicates that the problem is with the inability to shake components from the bundle. I don't see why a switch from classnames to clsx would enable rollup to shake hole components from the bundle.

I also added a bundle that used the build from this branch and it will still include clsx if I use import { colors } from "@material-ui/core"; .

@TrySound Could you provide an example that illustrates bundles with classnames are less "shakeable" than bundles with clsx?

[TrySound]

@eps1lon I said "assume material-ui is treeshakable". clsx will be treeshaked if it's used by a package based on hooks for example.

[TrySound]

Switching to clsx will allow to not spend a time to transpile classnames from cjs to esm and bytes with wrappers.

Why so much talking about switching to obviously better package? Why stuck with legacy because "everybody use it"?

[oliviertassinari]

Before moving forward, I think that we should consider the alternatives. Here is one classcat. Looking at their benchmark, clsx seems to be slower than classNames. Maybe we can find a better one?

[TrySound]

@oliviertassinari Probably because it does not access arguments. So it's not a drop-in replacement at all.

So stay with clsx?

[eps1lon]

Why so much talking about switching to obviously better package? Why stuck with legacy because "everybody use it"?

0. I wouldn't consider a 2M downloads/week package legacy if the new alternative has 100/week. With this sentiment the hole ecosystem becomes "legacy" every week.
1. usage numbers is one indicator for robustness
2. the more popular a package the more likely it is we don't add a new package to the bundle because it's already used by another one. This will 100% add a new package to applications looking at the current numbers of clsx. It's very like that most apps already use classnames so we don't actually bump bundlesize by using classnames
3. "better" needs to be defined and then weighted against "change" which always has some unknowns attached to it. So far the only verified arguments are isolated bundle size and micro benchmarks. The author himself said that the size is irrelevant here. Leaves the micro-benchmark and I'm currently missing how much this will actually mean for a render call. It's just not worth it if it's 1% of a render call. It may be worth it if it's 90% of a render call. Again without numbers I won't make that judgement call.
4. Since classnames and clsx have the same interface users could simply alias the import if they require those micro optimizations.

I don't think it's very hard to write a codemod that transforms every variadic call of classnames to a call of classcat with a single argument.

[TrySound]

material-ui-pickers has already migrated to clsx.

[mbrookes]

@oliviertassinari

Overall, we will increase people bundle. (#14152 (comment))

So it isn't just me that believes this then!

[rosskevin]

I'll decrease my bundle because we use clsx and use at least on library that uses clsx. This will be the last use of classnames for us.

Regardless, material-ui should choose the best library for the job. It is one of the most popular UI libraries in the world and our choices here signal good choices for others because they know we research and debate the small things.

LGTM.

[mbrookes]

we research and debate the small things.

In that spirit then 😇:

I'll decrease my bundle because we use clsx

Respectfully, that's perhaps the worst case of anecdata I've seen in a long while.

https://www.npmjs.com/browse/depended

classnames: 19/~450,000
clsx: ~449,000/~450,000

The odds of overlap are far from insignificant. However, as I said in chat, I'm willing to suck it up for the sake of a few hundred bytes. But lets not pretend the overhead isn't real (even if tiny) for a likely majority users.

[oliviertassinari]

https://twitter.com/MaterialUI/status/1092928715804299264

[oliviertassinari]

The best way to gather more data points around this change is to actually release it. We gain more user feedback with the alpha releases of v4. Also, I still hope we can completely get rid of this package in v4…

[juniorclarke]

Just tried to update to 4.0.0-alpha.0 to play with it but getting

./node_modules/@material-ui/core/esm/InputBase/Textarea.js Module not found: Can't resolve 'classnames'

Is there something I missed in the migration docs? Should I create a separate issue to track?

[juniorclarke]

So I looked at the diffs and just made the changes inline and it worked.

-import classNames from 'classnames';
+import clsx from "clsx";

and replaced classNames with clsx and my project loaded.

I'm guessing the next patch with fix this.

Thanks

[eps1lon]

@juniorclarke Thanks for reporting this. Fix is proposed: #14506

[eimanhakimy]

hello i'm beginner in react so can someone explain to me

What is className and function of className
What is clsx and function of clsx

[damianstasik]

@eimanhakimy without going into details:

• className is just HTML class attribute under the hood,
• clsx is a utility function that helps setting className value when a class/classes are applied conditionally.

[mbrookes]

For reference: https://github.com/lukeed/clsx#readme