Try: Replace classnames with clsx (outdated) #61091

DaniGuardiola · 2024-04-25T09:29:24Z

Important: superseded by #61138

What?

This PR attempts to replace classnames with clsx across Gutenberg, which is a faster and lighter alternative.

@tyxla already did this in calypso: Automattic/wp-calypso#89843

How?

It's a drop-in replacement. All classnames/dedupe instances were also safe to replace, except for one case where an obscure feature was being used - that was trivially replaced with equivalent code (see removeAspectRatioClasses commit for details).

Size gains

With classnames and classnames/dedupe we added 1.14kb (minified + gzipped) to the bundle. See:https://bundlejs.com/?q=classnames%402.5.1%2Cclassnames%402.5.1%2Cclassnames%402.5.1%2Fdedupe%2Cclassnames%402.5.1%2Fdedupe&treeshake=%5B*%5D%2C%5B%7B+default+%7D%5D%2C%5B*%5D%2C%5B%7B+default+as+d2+%7D%5D

With clsx, we only add 241B (minified and gzipped). See: https://bundlejs.com/?q=clsx%402.1.1&treeshake=%5B*%5D

That's about a kylobyte of savings.

github-actions · 2024-04-25T09:29:49Z

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
Labels found: .

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

github-actions · 2024-04-25T09:29:49Z

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
Labels found: .

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

github-actions · 2024-04-25T09:29:58Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

DaniGuardiola · 2024-04-25T09:37:09Z

Lots of CI failures that seems completely unrelated to the changes (basically a grep replace of classnames -> clsx.

Anyone know why this is happening? Is this just a side effect of touching a lot of repo-wide code?

tyxla · 2024-04-25T09:41:28Z

Lots of CI failures that seems completely unrelated to the changes (basically a grep replace of classnames -> clsx.

Anyone know why this is happening? Is this just a side effect of touching a lot of repo-wide code?

I'd suggest double-checking if it's really unrelated. There seems to be some relation at first glance:

DaniGuardiola · 2024-04-25T09:55:50Z

@tyxla most do seem unrelated but I also stumbled upon that one. I'm puzzled about that instance because I can't find what's being referenced there. Literally grepping for "classnames" case insensitive doesn't find any more relevant references in code. I feel like I'm missing something 🤔

tyxla · 2024-04-25T10:03:17Z

@tyxla most do seem unrelated but I also stumbled upon that one. I'm puzzled about that instance because I can't find what's being referenced there. Literally grepping for "classnames" case insensitive doesn't find any more relevant references in code. I feel like I'm missing something 🤔

Have you tried rebasing to the latest trunk? Seems like a classnames import was introduced there literally 40 mins ago in #60894.

DaniGuardiola · 2024-04-25T11:56:35Z

@tyxla that was it. Pushing fix. Thanks!

DaniGuardiola · 2024-04-25T12:02:38Z

Ok got some tests related to removeAspectRatioClasses failing, probably because of extra spaces in my naive replacement solution. Should be easy to remedy, fix incoming.

docs/contributors/code/coding-guidelines.md

DaniGuardiola · 2024-04-25T16:35:23Z

This is ready for review now. ~~Not sure why some PHP lints are failing, since no PHP has been touched 🤷~~ Found the issue, some automatic formatting of a PHP file slipped by. Not sure exactly how, but that's been reverted now.

mirka

I think we should include the restricted import eslint rule in this PR so nobody wastes their time figuring out what happened.

tyxla

This looks good and works well. 👍

I'd add some entries to all existing CHANGELOGs of affected packages since it's a change worth mentioning.

Since it's a change that affects the entire repo, perhaps we could wait for a couple of days before merging, in case someone has blocking feedback.

Thanks @DaniGuardiola 🙌

.eslintrc.js

youknowriad · 2024-04-25T21:22:33Z

Is there any reason why the bundle size comment is not visible on this PR, I wanted to check the impact.

I believe this needs to be announced in #core-editor at least to bring some visibility among contributors but other than that, it seems fine to land to me.

DaniGuardiola · 2024-04-26T07:43:14Z

@youknowriad

Is there any reason why the bundle size comment is not visible on this PR, I wanted to check the impact.

I'm curious too. Could someone more knowledgeable look into it? :)

I believe this needs to be announced in #core-editor at least to bring some visibility among contributors

Good point 👍 will make sure to do this after merge.

youknowriad · 2024-04-26T07:47:44Z

I'm curious too. Could someone more knowledgeable look into it? :)

It seems it's because you're using a fork. I saw that you already have 3 PRs merged into the repo so I added you to the Gutenberg team to be able to create branches and merge approved PRs.

If you want to check the bundle size impact before merging here, you'll have to recreate another PR using a branch in the main repo I guess.

DaniGuardiola · 2024-05-01T08:01:41Z

Closing this in favor of #61138 since I've continued work there.

DaniGuardiola added 2 commits April 25, 2024 11:12

Replace classnames with clsx.

d7f9139

Fix removeAspectRatioClasses.

7f8a794

fabiankaegy added the [Type] Code Quality Issues or PRs that relate to code quality label Apr 25, 2024

Fix docs.

201d0e3

Fix test.

99d68c1

Merge branch 'trunk' into pr/DaniGuardiola/61091

daac391

Fix trunk changes.

b5d9cd8

ellatrix reviewed Apr 25, 2024

View reviewed changes

docs/contributors/code/coding-guidelines.md Show resolved Hide resolved

Fix removeAspectRatioClasses.

30b3ce7

Undo unwanted PHP changes.

fb304a9

mirka reviewed Apr 25, 2024

View reviewed changes

Add restricted import rule.

58aced0

tyxla approved these changes Apr 25, 2024

View reviewed changes

.eslintrc.js Show resolved Hide resolved

DaniGuardiola mentioned this pull request Apr 26, 2024

Replace classnames with clsx #61138

Merged

DaniGuardiola closed this May 1, 2024

DaniGuardiola changed the title ~~Try: Replace classnames with clsx~~ Try: Replace classnames with clsx (outdated) May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try: Replace classnames with clsx (outdated) #61091

Try: Replace classnames with clsx (outdated) #61091

DaniGuardiola commented Apr 25, 2024 •

edited

Loading

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024 •

edited

Loading

DaniGuardiola commented Apr 25, 2024

tyxla commented Apr 25, 2024

DaniGuardiola commented Apr 25, 2024

tyxla commented Apr 25, 2024

DaniGuardiola commented Apr 25, 2024

DaniGuardiola commented Apr 25, 2024

DaniGuardiola commented Apr 25, 2024 •

edited

Loading

mirka left a comment

tyxla left a comment

youknowriad commented Apr 25, 2024

DaniGuardiola commented Apr 26, 2024

youknowriad commented Apr 26, 2024

DaniGuardiola commented May 1, 2024

Try: Replace classnames with clsx (outdated) #61091

Try: Replace classnames with clsx (outdated) #61091

Conversation

DaniGuardiola commented Apr 25, 2024 • edited Loading

What?

How?

Size gains

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024 • edited Loading

DaniGuardiola commented Apr 25, 2024

tyxla commented Apr 25, 2024

DaniGuardiola commented Apr 25, 2024

tyxla commented Apr 25, 2024

DaniGuardiola commented Apr 25, 2024

DaniGuardiola commented Apr 25, 2024

DaniGuardiola commented Apr 25, 2024 • edited Loading

mirka left a comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

youknowriad commented Apr 25, 2024

DaniGuardiola commented Apr 26, 2024

youknowriad commented Apr 26, 2024

DaniGuardiola commented May 1, 2024

DaniGuardiola commented Apr 25, 2024 •

edited

Loading

github-actions bot commented Apr 25, 2024 •

edited

Loading

DaniGuardiola commented Apr 25, 2024 •

edited

Loading