Support Hot Module Replacement (HMR) for CSS #64444
Draft
+463
−36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevin940726
added
[Type] Build Tooling
|
Size Change: +8.89 kB (+0.5%) Total Size: 1.79 MB
ℹ️ View Unchanged
|
kevin940726
changed the title
swissspidy
reviewed
Aug 13, 2024
|
Flaky tests detected in a51dfd4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10423728271
|
gziolo
reviewed
Aug 19, 2024
There was a problem hiding this comment.
Nice one. There are a few small tweaks we could land today to the current configuration. It would be a huge win to be able to st CSS styles updated on the fly for WP packages. I'm wondering if it will fully work for the styles that get also included in the iframed editor, as they might require another layer that automatically syncs changes to the editor's iframe. Edit: I realized that there is a glue layer that iterates over all frames to update CSS files 👍🏻
lib/client-assets.php
Outdated
| // Register the runtime script. | ||
| gutenberg_override_script( | ||
| $scripts, | ||
| 'wp-runtime', |
There was a problem hiding this comment.
Do you think it should be included in the WordPress core, too? Otherwise, it could be a regular script registration and the script handle should be more gutenberg-packages-runtime.
There was a problem hiding this comment.
What do you mean by a regular script registration? I'm not that familiar with PHP script registration and loading 😅.
There was a problem hiding this comment.
If it isn’t included in WP core, there is no value in the overhead of gutenberg_override_script which unregisters core scripts first.
There was a problem hiding this comment.
I see. I think this is worth getting into core too. I need to get familiar with core's webpack config first though 😅.
| @@ -275,7 +277,7 @@ | |||
| "build:plugin-zip": "bash ./bin/build-plugin-zip.sh", | |||
| "clean:package-types": "tsc --build --clean && rimraf \"./packages/*/build-types\"", | |||
| "clean:packages": "rimraf \"./packages/*/@(build|build-module|build-style)\"", | |||
| "dev": "cross-env NODE_ENV=development npm run build:packages && concurrently \"wp-scripts start\" \"npm run dev:packages\"", | |||
| "dev": "cross-env NODE_ENV=development npm run build:packages && concurrently \"wp-scripts serve\" \"npm run dev:packages\"", | |||
There was a problem hiding this comment.
What are the limitation of wp-scripts start --hot that force introducing a new script? I would be more in favor of always using the dev server for start by default even when not using hot module replacement.
There was a problem hiding this comment.
We want to use our own manual HMR setup so we don't want to pass --hot to webpack. wp-scripts start --hot seems to always pass that to webpack which then override the hot: false in the config.
I agree using start will be ideal but serve seems reasonable too, given that the official webpack command uses serve to start a dev server too.
There was a problem hiding this comment.
Sure, webpack serve is a thing which is reflected in wp-scripts start implementation. The same applies to webpack watch but it doesn’t need to be exposed to the devs.
| }, | ||
| liveReload: false, | ||
| devMiddleware: { | ||
| writeToDisk( filePath ) { |
There was a problem hiding this comment.
That's something we could reuse in the default config shipped with @wordpress/scripts as we don't need to save these files on disk that are specific for hot module replacement.
gutenberg/packages/scripts/config/webpack.config.js
Lines 311 to 327 in 31e9078
There was a problem hiding this comment.
In fact, we don't actually need to write anything to the disk 😆. The only reason we do now is because the PHP backend uses glob to search for the built files on the disk, which I think is probably an anti-pattern we can avoid? Practically we want to avoid the calls for IO since it's often the most costly operations, I guess we can replace that with a generated manifest JSON and/or something like a API call in development mode. I try to keep everything the same in this PR as much as possible to prevent any accidental breakage though 😅.
There was a problem hiding this comment.
There are also asset files produced (in core it’s a single file, maybe in Gutenberg, too) which work like a manifest. Anyway, if you see some room for improvement you probably should look at optimizing WP core, too.
gziolo
reviewed
Aug 19, 2024
| @@ -143,29 +152,103 @@ module.exports = { | |||
| performance: { | |||
| hints: false, // disable warnings about package sizes | |||
| }, | |||
| optimization: { | |||
| ...baseConfig.optimization, | |||
| runtimeChunk: { | |||
There was a problem hiding this comment.
Does it make sense to enable the runtime chunk also on production? I was wondering whether it's something that we should also add to the @wordpress/scripts if there is more than a single chunk to ensure it works correctly with React Fast Refresh. However, my initial thinking was it only is useful in the dev mode when using hot module reloading.
There was a problem hiding this comment.
Generally, we want to use a single runtime chunk when we have multiple entries in webpack. The size of the runtime is minimal in production though, and it doesn't seem to impact the overall bundle sizes that much (see #64444 (comment)). Extracting the runtime makes it easier to integrate with webpack features like HMR and code splitting though. I think it makes sense to do it in both development and production mode.
kevin940726
force-pushed
the
add/css-hmr
branch
2 times, most recently
from
92f9db5
to
b25200b
Compare
kevin940726
force-pushed
the
add/css-hmr
branch
from
b25200b
to
db6307b
Compare
|
Do we still need a backport PR if this is intended to use in gutenberg plugin only? IIUC, core only uses the npm published packages, so this won't affect them. However, I think it's beneficial to also explore the same setup in core, in other PRs of course. |
|
Is this PR still relevant? If so, is there someone that would care to take it over? |
ramonjd
added
the
No Core Sync Required
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Part of #36142.
Add Hot Module Replacement for CSS. This is an experimental development-only feature.
Why?
HMR for JavaScript is a little bit more complicated and supporting CSS also helps pave the way too.
Hot Module Replacement (or hot reload) is a bundler (webpack) feature that replaces the module upon saving the files without reloading the page. This PR adds support for CSS HMR so that updating CSS files locally automatically updates the stylesheets on the page in development mode. This helps developers iterate more quickly with faster visual feedback.
How?
Due to how Gutenberg works, this is a bit complicated but I'll try my best explaining it 😅.
To understand how HMR works, we must understand the current way of building CSS in Gutenberg. When running
npm run dev, there are multiple steps to complete to update the CSS on the page:flowchart LR A(packages/*/src/style.scss) -->|SASS compiler / postCSS| B(packages/*/build-style/style.css) B -->|webpack| C(build/*/style.css)build-workerbuilds the SASS source files to CSS files to each package'sbuild-stylefolder. These files will be published to the npm registry.builddirectory for the page to consume.We don't change the first step in this PR to simplify and minimize the scope (but we could for future optimizations).
Now, let's move on to how HMR works in webpack. The official doc has a great overview of the technical background. The difference between the recommended setup and our setup are:
The first difference is relatively easy to fix if we just unshift the style entries for each JavaScript entry.
This tells webpack to load the styles in each JavaScript entry so that they can be processed by webpack and inject the HMR runtime. We want to keep the JavaScript file at the end so that they'll be exported to the
window.wpproperty.The second difference is important because we don't want to change the order of the styles to prevent breakage. This means we can't use
style-loader, which is the most popular way to inject and hot reload CSS files at the time.Instead, we create a separate custom loader for CSS files that includes the HMR code to replace injected styles on the fly. I first tried out
restyle-loaderwhich mostly did what we want but it lacks maintenance and doesn't work for iframed styles.css-hmr-loaderis refactor that largely based onrestyle-loaderandmini-css-extract-pluginthat suits our needs. The code uses the HMR runtime API to listen and react to code changes.The next step is to start a dev server that listens to the HMR event on the client.
webpack-dev-serverprovides everything we need, we just need to provide a manual entry for the HMR runtime because we have multiple entry points. The dev server will listen on port8887which matches the port used in blocks development.Setting the dev server up also means we no longer treat the file system as the source of truth, but rather the hot-enabled in-memory dev server. Any request to the original URL (
http://localhost:8888/wp-content/plugins/gutenberg/build/*) will need to be redirected to the dev server (http://localhost:8887/build/*) for HMR to work. We achieve this by changing the implementation ofgutenberg_urlinclient-assets.php. I'm not sure if that's enough, if not then we could also go a bit further and inject a custom.htaccessfile.As a bonus, but also pretty essential, is that we also want to continue to support running e2e tests in development mode. Since there's no difference from the PHP backend perspective to know whether it's loading a production build or a hot-enabled development mode, the most reliable way is to check whether a
hot.jsentry point is created in the build folder. If it exists then we assume it's running a development build, redirecting fromlocalhost:8888tolocalhost:8887.I believe this covers most of the implementation in this PR. Feel free to ask any further questions or if you spot any bugs!
The same implementation could and should be able to be used for JavaScript HMR too. But I think that's better left to another PR to minimize the scope 😅.
Testing Instructions
npm run dev(runrm -rf buildbeforehand if you want to test it from scratch.).cssfiles and see if they reflect themselves on the page without a reload.localhost:8889and production buildsnpm run build. Expect they still work correctly.Screenshots or screencast
Kapture.2024-08-13.at.11.30.56.mp4