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

feat: make css-modules class names human-readable #143

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

Pistch
Copy link
Contributor

@Pistch Pistch commented Jun 17, 2024

Minor improvement to css modules support.
This pull request makes generated css-modules class names human readable (instead of something like kImIF7sZoAjSPn74Hpe8).

Without this improvement css-modules are nearly unusable and are completely undebuggable.

===============
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en.

Comment on lines 502 to 509
...(isCssModuleRules
? {
modules: {
localIdentName: '[name]__[local]--[hash:base64:5]',
exportLocalsConvention: 'asIs',
},
}
: {}),
Copy link
Collaborator

@ValeraS ValeraS Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use these options and no other changes will be required.

Suggested change
...(isCssModuleRules
? {
modules: {
localIdentName: '[name]__[local]--[hash:base64:5]',
exportLocalsConvention: 'asIs',
},
}
: {}),
modules: {
auto: true, // uses modules only with files *.module.s?css
localIdentName: isEnvDevelopment
? '[name]__[local]--[hash:base64:5]'
: '[hash:base64]',
exportLocalsConvention: 'camelCase', // gives both camel case and as is
},

Copy link
Contributor Author

@Pistch Pistch Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you think readable class names are useful for non-dev environments as well?
I see no harm in exposing those in production builds.
This approach is hardly different from using BEM-naming.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We minify all other code except class names, which seems strange to me. And here are source-maps to see original classnames.

Copy link
Contributor Author

@Pistch Pistch Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we don't minify BEM classnames. I really doubt we minify css custom property names.

If we were to compare those side-by-side, the difference to total file size seems negligeable:

.MnET1WGLIieyGuzfo5cw {}
.SomeComponent_wrapper-f1da4 {}

Hashes generally have more entropy than variable (or file) names, which leads me to suggest total amount of bytes transferred over network (after gzip/brotli) will be even less for readable names. Didn't measure it, ofc :)

Copy link
Contributor Author

@Pistch Pistch Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example why source maps do not help in this case.
How can i attribute those classes to something in my code?
Screen Shot 2024-06-18 at 15 14 43

It is possible to go inside "sources" tab and find something, provided you deployed source maps to static cluster along with bundled code.

@Pistch Pistch force-pushed the fix-modules-classnames branch from a55a94c to dcc0281 Compare June 18, 2024 08:36
@Pistch
Copy link
Contributor Author

Pistch commented Jun 20, 2024

Hey? :]

@ValeraS ValeraS merged commit e324a8b into gravity-ui:main Jun 20, 2024
2 checks passed
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