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

[Performance]: Replace classnames usage with clsx #47760

Merged
merged 10 commits into from
May 31, 2024

Conversation

nefeline
Copy link
Contributor

@nefeline nefeline commented May 23, 2024

Submission Review Guidelines:

Changes proposed in this Pull Request:

Replace classnames with clsx across WooCommerce, which is a faster and lighter alternative.

This is aligned with the equivalent recent change made on Gutenberg on WordPress/gutenberg#61138

Closes #47602

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

No visible changes should be observed to the end user.

  1. Make sure all tests on this PR pass.
  2. Do a quick smoketest with a few Woo blocks & confirm there are no visual regressions.
  3. Do a quick smoketest by accessing WooCommerce > Home > Customize Your Store and confirm there are no visual regressions.

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Replaced classnames package with the faster and smaller clsx package.

Comment

@nefeline nefeline linked an issue May 23, 2024 that may be closed by this pull request
@github-actions github-actions bot added the focus: monorepo infrastructure Issues and PRs related to monorepo tooling. label May 23, 2024
@nefeline nefeline changed the title Replace classnames with clsx within woocommerce-blocks. [Performance]: Replace classnames usage with clsx May 23, 2024
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label May 23, 2024
Copy link
Contributor

github-actions bot commented May 23, 2024

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

@nefeline nefeline marked this pull request as ready for review May 24, 2024 11:47
@woocommercebot woocommercebot requested review from a team and thealexandrelara and removed request for a team May 24, 2024 11:47
@nefeline nefeline requested review from a team and kmanijak and removed request for thealexandrelara and a team May 24, 2024 11:47
Copy link
Contributor

github-actions bot commented May 24, 2024

Hi @kmanijak, @woocommerce/woo-fse

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@nefeline nefeline requested a review from a team May 24, 2024 11:47
@samueljseay
Copy link
Contributor

Just a heads up, something we should do on all these PRs (we dont have automation yet for, but should) is run pnpm analyze-bundles and compare against trunk, specifically we're most interested in frontend and main bundles I believe it should be smaller but we really should validate just in case 😄 I think its also worth mentioning that classnames is only 360B gzipped in both of our bundles right now, so it's not really very big. Would be good to compare the gzipped size of clsx in the bundle for confirmation

@samueljseay
Copy link
Contributor

Oh and you'll also be able to validate if classnames was actually removed as a dependency :) because there might be underlying deps relying on it it might not change bundle size (or could increase it, although if clsx runtime perf is better it might be worth!)

@samueljseay
Copy link
Contributor

samueljseay commented May 24, 2024

Screenshot 2024-05-24 at 8 02 42 PM Screenshot 2024-05-24 at 8 02 57 PM

Ah yeah confirmed my suspicion that classnames is not gone. So this will increase the bundle size (for now). I can't speak for runtime perf so I'll leave that up to you. this is a very small impact, it just won't reduce bundle size right now.

EDIT, looked at the runtime perf link, looks good to me and worth the change imho!

@nerrad
Copy link
Contributor

nerrad commented May 26, 2024

Likely, won't be completely gone until the upstream WP packages that include it are also released with the changes.

With that said, along with what Sam mentioned, it'd be good to identify what external packages are pulling in classnames in case there are any that have updates (or might need alternative if we want to eliminate usage). It's likely it'll just be WP packages needing updated though (when they are released).

@nefeline
Copy link
Contributor Author

nefeline commented May 30, 2024

@samueljseay @nerrad @danielwrobert thanks for the reviews & my apologies for the slight delay as I had to switch focus to other higher priorities + had an AFK this week :)

Just a heads up, something we should do on all these PRs (we dont have automation yet for, but should) is run pnpm analyze-bundles and compare against trunk, specifically we're most interested in frontend and main bundles I believe it should be smaller but we really should validate just in case 😄 I think its also worth mentioning that classnames is only 360B gzipped in both of our bundles right now, so it's not really very big. Would be good to compare the gzipped size of clsx in the bundle for confirmation

@samueljseay thanks for the suggestion! I see you already opened a PR for the automation so that's great: #47880 looking forward to seeing it in place 🎉 .

I did compare the bundle sizes on trunk versus on this branch and it is slightly bigger indeed:

This branch (with clsx) Trunk
Parsed size: 236.65 Parsed size: 236.25
Gzipped size: 76.64 Gzipped size: 76.52
Stat size: 783.79 Stat size: 783.42
Screenshot 2024-05-30 at 14 19 36 Screenshot 2024-05-30 at 14 27 21

Given that:

this is a very small impact, it just won't reduce bundle size right now.
looked at the runtime perf link, looks good to me and worth the change IMHO!

How about we merge the changes here and work on additional PRs/steps to investigate reducing the size of the build separately? This PR is already rather big and the runtime performance improvement is worth the change, not to mention it is aligned with what Gutenberg is doing.

@samueljseay
Copy link
Contributor

@nefeline more than happy to merge this based on runtime perf benefits alone! 🚢

@nefeline nefeline merged commit 5dd7713 into trunk May 31, 2024
97 of 98 checks passed
@nefeline nefeline deleted the 47602-performance-replace-classnames-usage-with-clsx branch May 31, 2024 03:49
@github-actions github-actions bot added this to the 9.1.0 milestone May 31, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label May 31, 2024
@rodelgc rodelgc added status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Jun 5, 2024
thealexandrelara pushed a commit that referenced this pull request Jul 18, 2024
* Replace classnames with clsx within woocommerce-blocks.

* Undo unnecessary change to getClassnames const.

* Replace classnames with clsx within woocommerce-admin.

* Add changelog.

* Update the pnpm lock file

* Address lint.

* Address lint errors for the block-library.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: monorepo infrastructure Issues and PRs related to monorepo tooling. plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Performance]: Replace classnames usage with clsx
5 participants