-
Notifications
You must be signed in to change notification settings - Fork 2k
Add support for CSS modules with SSR consistency #103606
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
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~286 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~46 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~4 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, and I've confirmed that the output is stable as expected.
Didn't mean to nitpick the CSS — I don't think we necessarily need to clean up every possible thing when converting existing code, but just left some thoughts in case it adds to the pile of patterns that can be simplified when converted to modules. In fact, for "quick" conversions, it may actually be safer and efficient to maintain the existing cascade/specificity as much as possible.
{ __( 'Get Started' ) } | ||
</Button> | ||
) } | ||
<ThreeColumnContainer> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was almost about to say we should keep the ThreeColumnContainer
in sync for the EducationFooter
and MarketplaceFooter
, but looking at the actual components they look pretty unrelated. Probably better that they're separated now.
--color-accent-60: var( --studio-blue-60 ); | ||
margin-bottom: -32px; | ||
|
||
& .marketplace-footer__section::before { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .marketplace-footer__section
selectors don't need to be nested anymore since we have our own unique class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
.marketplace-footer__section
selectors don't need to be nested anymore since we have our own unique class.
While that's true, we have an issue that these styles are meant to override the default styles applied by the Section
component. If we un-nest this, the selectors will have equal specificity, which may mean that the override doesn't take effect.
A few thoughts on how we could address this:
- Normally I might want to do something like
.marketplace-footer__section.section::before
to both explicitly make it clear that we're overriding styles from.section
and increase the specificity, but sinceSection
uses Emotion, we don't have a stable selector to use. - Ideally we wouldn't be overriding through styles like this in the first place, and
Section
would allow (or not allow) what types of customizations it expects to be permitted for the background color. In a way, it could be argued that it's "by design" that the background isn't allowed to be overridden through styles like this, and this is part of what we hope to achieve with using CSS modules. - Absent the other options, we could leave this as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm inclined to leave this as-is for the purpose of this pull request, and follow-up with an enhancement to the <Section />
component to better support additional variations of background color.
background-color: #f6f7f7; | ||
} | ||
|
||
&:not(.is-logged-in) .marketplace-footer__section { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the modifier could've been set directly on marketplace-footer__section
and not on marketplace-footer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the modifier could've been set directly on
marketplace-footer__section
and not onmarketplace-footer
.
This sent me down a rabbit hole on trying to determine if modifier classes are valid on BEM Elements 😂 Since I usually only see them paired with Blocks. But it seems to be perfectly valid!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the modifier could've been set directly on
marketplace-footer__section
and not onmarketplace-footer
.
Updated in fd65ebf.
9a99e61
to
fd65ebf
Compare
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
createHash( 'md5' ) | ||
.update( context.resourcePath + localName ) | ||
.digest( 'hex' ) | ||
.substr( 0, 10 ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently generates completely opaque class names that begin with _
:
const style_module = ({
"marketplace-footer": `_5df97c7b40`,
"marketplace-footer__section": `_3c3498323c`,
"is-logged-in": `_d93f9bf602`,
"marketplace-footer__cta": `_92b887b139`,
"marketplace-footer__three-column": `_bf9c1e28f8`
});
Could we make the styles (and the DOM markup that the React app generates) more human readable by adding the hash only as a suffix to the original name? The class names then would be like
marketplace-footer_5df97c7b40
Without this, inspecting DOM markup in devtools would be almost impossible: everything would be a div
with unreadable class names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To some extent, the opaqueness is the point, since I believe that one of the goals of using CSS modules is to close components for external extension in order to avoid CSS (class names, etc.) being considered as part of a component's public API for backwards-compatibility.
Using the recommendation you suggested, it would open up the possibility of people doing something like...
.my-component [class^="marketplace-footer_"] {
/* ...overrides */
}
But I agree it does have some trade-offs, and I've even caught myself being confused by the DevTools DOM inspector structure trying to find where styles are applied.
A couple thoughts:
- Maybe we don't need to be so strictly rigid here, and someone overriding like above is either unlikely or requires some tacit understanding of the overrides being fragile
- We could change the behavior to fully mangle in production builds and have more-helpful names in development. This could be error-prone however, since code like above would work in development and fail when deployed to production. Additionally, it could be helpful to maintain some debuggability in production.
- If mangling names is beneficial for backwards-compatibility, maybe we limit it to applying to situations where we need to more carefully consider backwards-compatibility (
packages/components
, or packages more broadly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea is that we could have Stylelint check and lint against the user of [class^=...]
. It's pretty uncommon and almost always a code smell to at least lint by default and require explicit override to disable.
I think I'll incorporate the component basename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated in 9ff1d89 to incorporate the original identifier, so now we get something like:
const style_module = ({
"marketplace-footer": `marketplace-footer_de608`,
"marketplace-footer__section": `marketplace-footer__section_f95a5`,
"is-logged-in": `is-logged-in_91412`,
"marketplace-footer__cta": `marketplace-footer__cta_12292`,
"marketplace-footer__three-column": `marketplace-footer__three-column_d7cfb`
});
One other thing I noticed is that we want the string to be deterministic so that it's the same between client and server builds, but not too deterministic that the class name will include a hash that is relatively stable over time (e.g. allowing someone to just target .marketplace-footer_de608
for overrides).
Open to suggestions, but I opted to try to detect the current git HEAD SHA value and use that as part of the generated hash. If it's unavailable for any reason (e.g. static file copy of the project), it'll gracefully fall back to creating a hash like hash.update( 'marketplace-footerundefined' )
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a good compromise. We can always revisit at a later point (especially given that it's not meant to be a public API contract).
I also like the idea of a Stylelint check, although we need to make sure not to create too many false positives.
Demonstrate functional server-rendered CSS modules behavior
Also use git head sha (if available) to have value be deterministic, yet unstable
fd65ebf
to
9ff1d89
Compare
Part of ARC-85
Related to / builds upon #103089
Proposed Changes
MarketplaceFooter
)Our default Webpack configuration already has support for CSS modules because it is enabled by default in
css-loader
, but it requires a few additions:css-loader
generates a unique salt that can produce different results between builds. Therefore, we overridegetLocalIdent
to produce a stable resultexportOnlyLocals
and disableMiniCssExtractPlugin
Why are these changes being made?
MarketplaceFooter
fromEducationFooter
: the previous location had two component definitions serving two distinct purposesTesting Instructions
Verify no visual difference in the marketplace footer
Ensure that generated class names are consistent:
yarn build-server
yarn build-client
grep marketplace-footer__three-column build/server.js
"marketplace-footer__three-column":
_02216b3f2e``grep -R marketplace-footer__three-column public/evergreen
\"marketplace-footer__three-column\":\"_02216b3f2e\"
Pre-merge Checklist