-
Notifications
You must be signed in to change notification settings - Fork 2k
StepContainerV2: Migrate to CSS modules #103089
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 (~543 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~4310 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 (~9082 bytes removed 📉 [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. |
002a88c
to
6c7a323
Compare
{ | ||
test: /\.(sc|sa|c)ss$/, | ||
loader: 'ignore-loader', | ||
}, | ||
scssConfig.loader, |
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.
Could/should we still ignore everything except files suffixed with .module.scss
?
Why were we not doing [server-side CSS processing and extraction] before? Is there anything we should do differently?
I'm not sure exactly why we would have needed to do it previously. Am I correct in understanding that it's required now so that the server can generate the computed class names in the server-rendered markup? Since we would have previously had static class names, I would suppose that's the reason we wouldn't need to process the stylesheets. I'd imagine there's also a performance overhead in doing so, at least in contrast with ignoring them altogether.
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.
Could/should we still ignore everything except files suffixed with .module.scss ?
Interesting. I think that would help with performance.
Am I correct in understanding that it's required now so that the server can generate the computed class names in the server-rendered markup?
Yes, that is exactly why. The class names must match both in the client and the server-generated markup.
@@ -18,6 +18,29 @@ module.exports.loader = ( { includePaths, prelude, postCssOptions } ) => ( { | |||
loader: require.resolve( 'css-loader' ), | |||
options: { | |||
importLoaders: 2, | |||
modules: { | |||
auto: /\.module\.scss$/, // Only enable CSS modules for .module.scss files | |||
getLocalIdent: ( context, localIdentName, localName ) => { |
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.
What are we trying to achieve with this behavior?
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 is needed for deterministic class name generation between the server and the client, since each Webpack entry processes its own chunk.
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 is needed for deterministic class name generation between the server and the client, since each Webpack entry processes its own chunk.
Interesting. That makes sense to me, though I would have thought the default behavior would produce a deterministic identifier as well.
One of the concerns I had is whether this could enable someone to continue targeting external CSS styles at the generated name, and if that's something we'd want to be discouraging.
e.g.
[class^="StepContainerV2_style-module-scss__"] {
// ...some override
}
Since the default behavior for CSS module class name generation doesn't have anything static to target, it would not be possible.
|
||
// Custom class name for modules | ||
return ( | ||
context.resourcePath.split( '/' ).slice( -2 ).join( '_' ) + |
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.
Do we have any guarantees that this will produce safe CSS 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.
resourcePath
are valid file names, so I'm guessing so.
However, I think we can go with the simpler solution following the hashing in this file:
require( 'crypto' )
.createHash( 'md5' )
.update( context.resourcePath + localName )
.digest( 'hex' )
This seems to be enough.
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.
There are some weird gotchas like not allowing to start with a number which could happen with a file name or a hash value.
Looks like the default behavior has some extra checks to handle this:
The server doesn't need to know any CSS because it's not a part of the markup it outputs. The markup is only HTML/JSX elements, with class names. The server only needs to know which CSS/JS assets are associated with a certain page (section) like The only reason why the server needs to start caring about CSS now is that the CSS modules export the actual class names. The class name in the server-generated markup is no longer a hardcoded The server continues to not care about the actual content of the CSS rules, it's interested only in the class names. |
@jsnajdr so you're saying we should be fine with loading the CSS and generating the class names, but instead of extracting the CSS ( |
Yes, we certainly don't want the server build to output any CSS assets because it won't use them. CSS is built as part of the client build (which is isomorphic to server). And the server then embeds the client CSS assets as We want webpack to compile CSS modules into something like: // this is the `style.scss` module transformed into JS:
const styles = {
'step-container-v2__content-row': 'abcd123',
'left': 'efgh456',
'right': 'ijkl789',
}
export default styles; I.e., export the classnames, but nothing else, particularly not the content of the CSS rules. I don't know right now which configuration of the various CSS loaders achieves this. We also need to make sure that the generated class names ( |
I guess that would imply:
|
@ciampo The hashing algorithm in the
That means that with little care, we should be able to guarantee identical scrambling both for client and server. We don't need to replicate the entire environment to the last detail. Also, the |
Proposed Changes
Migrates StepContainerV2 to CSS modules instead of static CSS class names to prevent overrides.
On several occasions, we observed PRs trying to achieve visual goals by modifying the wireframe or container styles. This isn't supposed to be like that. If we let these changes in, then the whole purpose of StepContainerV2 (consistency, predictability, composability) goes down the drain.
This PR changes from static CSS to CSS modules to prevent class names and avoid such scenarios. It does so by modifying the Webpack configuration so it supports
.module.scss
files and generates class names, even on the server, as some wireframes (Loading and, very soon, CenteredContentLayout) are rendering styles on the server.This will also make #102503 irrelevant, as the problem this PR solves will solve that one as a side effect.
Reviewing this PR
@Automattic/team-calypso, you can disregard all changes within
packages/onboarding
. The meat of this PR is withinpackages/calypso-build/webpack/sass.js
andclient
.Before, the server bundle processing completely ignored CSS. Now, both SSR and CSR modes include CSS processing and extraction. Why were we not doing that before? Is there anything we should do differently?
Testing Instructions
Ensure you see no layout shifts and that the steps are properly applied when entering
/setup
, including the centered loading bar and the WordPress logo at the top-left corner.