CSS Modules HMR working again #3031

justin808 · 2021-05-26T02:57:17Z

Demo: https://youtu.be/lIkhnLXIH6I

Changes

Enable CSS modules HMR if webpack-dev-server running and HMR is on. Enabled by using the style-loader rather than extract CSS
Add docs for developing packages
export inliningCss from JS package
Remove inject_client webpacker.yml setting as this can be set by the HMR value
Added setting of injectClient and injectHot per the HMR setting
Simplifications and fixes for HMR
1. HMR default to true for webpacker.yml
2. inline, injectClient, and injectHot default to the value of hmr
3. hmr turned on for the webpack dev server if the hmr config is true
4. using command line option for --hot as the docs recommend that over
  the plugin.

To Test

Tested on shakacode/react_on_rails_demo_ssr_hmr#12

Checkout branch justin808/change-css-loader
bundle && yarn
Run foreman start -f Procfile.dev-hmr
Open up http://localhost:3000/hello_world and keep it visible to see the hot reloading
Edit the string on line 17 of HelloWorld.tsx and see the string change in the browser change upon saving the file.
Edit the text color on line 2 of HelloWorld.module.css and see the browser change upon saving the file.

justin808 · 2021-06-02T07:31:12Z

@gauravtiwari can you give me a review, please?

dhh · 2021-06-02T09:03:07Z

lib/webpacker/helper.rb

@@ -137,7 +137,9 @@ def preload_pack_asset(name, **options)
  #   <%= stylesheet_pack_tag 'calendar' %>
  #   <%= stylesheet_pack_tag 'map' %>
  def stylesheet_pack_tag(*names, **options)
-    stylesheet_link_tag(*sources_from_manifest_entrypoints(names, type: :stylesheet), **options)
+    unless current_webpacker_instance.dev_server.running?


This seems risky. Must avoid ever having a path that leads to a port check happening in production. Seems like something that should happen outside the pack call itself.

@dhh, does the refactoring clarify that there cannot be a port check during production?

justin808

@dhh Please let me know if I addressed your concerns. I also improved the HMR setup.

justin808 · 2021-06-03T10:13:36Z

lib/install/config/webpacker.yml

-    hmr: false
-    # Inline should be set to true if using HMR; it inserts a script to take care of live reloading
-    inline: true
+    hmr: true


I see no reason to have the extra settings in the YML file since they are not used by the Ruby code and they can be customized in a custom Webpack config.

justin808 · 2021-06-03T10:14:22Z

lib/webpacker/dev_server.rb

@@ -11,7 +11,7 @@ def initialize(config)
    @config = config
  end

-  def running?
+  def configured_and_running?


By renaming this method, it's clear that there's no cost for checking if the devServer is "configured" on a production environment.

I know I flagged this, but seeing it in context, it actually looks worse. I think we were better off with just #running?. Which is kinda ironic because I vaguely remember flipflopping on this exact question when we originally made web packer!

justin808 · 2021-06-03T10:14:52Z

lib/webpacker/dev_server_runner.rb

@@ -30,6 +30,7 @@ def load_config
        @port              = dev_server.port
        @pretty            = dev_server.pretty?
        @https             = dev_server.https?
+        @hot               = dev_server.hmr?


Recent docs on using HMR with the webpack-dev-server all suggest using the command line option rather than the plugin.

justin808 · 2021-06-03T10:15:44Z

lib/webpacker/railtie.rb

@@ -8,7 +8,7 @@ class Webpacker::Engine < ::Rails::Engine
  config.webpacker = ActiveSupport::OrderedOptions.new

  initializer "webpacker.proxy" do |app|
-    insert_middleware = Webpacker.config.dev_server.present? rescue nil


There is no need to rescue and swallow errors here.

Don't think the local variable explains anything. Should just inline into the conditional.

justin808 · 2021-06-03T10:16:21Z

package/environments/development.js

-      injectClient: devServer.inject_client,
+      inline: devServer.inline || devServer.hmr,
+      injectClient: devServer.hmr,
+      injectHot: devServer.hmr,


Let's base the default "it just works" settings based on the one hmr setting.

justin808 · 2021-06-03T10:18:34Z

package/utils/helpers.js

@@ -39,6 +39,9 @@ const canProcess = (rule, fn) => {
  return null
 }

+const runningWebpackDevServer = process.env.WEBPACK_DEV_SERVER &&
+  process.env.WEBPACK_DEV_SERVER !== 'undefined'


When running the webpack-dev-server, this ENV value is set.

rossta · 2021-06-03T17:40:27Z

Thanks @justin808 for working on getting CSS module HMR working. I understand it's an important feature to projects like react_on_rails and I appreciate your efforts here.

I do feel that changing the default to orient toward HMR should be reconsidered. My strong personal preference is that extracting css in development and test should be the default, even if the dev server is running. That means I also prefer that HMR should not be enabled by default.

As we saw in last year's May-of-WTFs, Webpacker has not provided a great out-of-the-box experience for many Rails developers. Anything we can do improve the OOB experience would be a win in terms of adoption and getting more folks helping improve the project. Extracting CSS and disabling HMR, even if the dev server is running, is closer to what I expect newcomers would expect and decrease the likelihood of those WTF moments that turn developers off. When extract_css used to be false by default, this lead to a lot of reported issues that boiled down to lack of understanding. Enabling HMR makes sense for advanced users who know how to leverage its benefits but, I believe, creates confusion for folks encountering tools like webpack and Webpacker for the first time.

To summarize, I'd prefer the view helper logic look something like

extract_css = !Webpacker.dev_server.running_and_hmr_enabled? # not necessarily the suggested method name

And in config/webpacker.yml for development.

hmr: false

justin808 · 2021-06-04T09:18:15Z

I do feel that changing the default to orient toward HMR should be
reconsidered. My strong personal preference is that extracting css in
development and test should be the default, even if the dev server is running.
That means I also prefer that HMR should not be enabled by default.

The big question is:

If you want simplicity and lack of WTFs, why not use bin/webpack --watch and see that webpack generated the static (actual) files upon saving changes?

Is there another benefit to using the webpack-dev-server without HMR? Is it any faster than watch mode?

Maybe we can make the default for development mode to be bin/webpack --watch and leave HMR as the default if somebody wants to run bin/webpack-dev-server?

For tests, static files are the way to go.

As we saw in last year's May-of-WTFs, Webpacker has not provided a great
out-of-the-box experience for many Rails developers. Anything we can do improve
the OOB experience would be a win in terms of adoption and getting more folks
helping improve the project. Extracting CSS and disabling HMR, even if the dev
server is running, is closer to what I expect newcomers would expect and
decrease the likelihood of those WTF moments that turn developers off. When
extract_css used to be false by default, this lead to a lot of reported issues
that boiled down to lack of understanding. Enabling HMR makes sense for
advanced users who know how to leverage its benefits but, I believe, creates
confusion for folks encountering tools like webpack and Webpacker for the first
time.

I agree with "Anything we can do improve the OOB experience." That's why we should remove extra devServer options in the config/webpacker.yml that can be easily set in the JS files for the webpack configuration. To this point, having one development.devServer.hmr = true option that controls everything to make HMR function is a big win.

I agree that having a default of CSS extraction is a big win, as that is the typical scenario of both bin/webpack --watch and production.

However, JS devs would expect that HMR is on by default, as that's the default for creating-react-app, gatsby, and next.js.

So not having HMR work by default would be a WTF for others. Having finicky things like HMR "it just works" is a big reason to use a library like rails/webpacker.

So can we have:

bin/webpack --watch default for development and testing
bin/webpack-dev-server for advanced JS devs with features like HMR.

I'm also OK with @rossta's idea of adding the check that HMR is on before disabling the CSS extraction and style tag. At the same time, I don't see the point of using the webpack-dev-server without HMR, when it seems much simpler to just use bin/webpack --watch.

Any opinions?

justin808 · 2021-06-09T08:31:23Z

Per my previous comment, there might be a slight performance advantage in using the webpack-dev-server per these docs.

So I'm OK with having HMR be off for the default, and if HMR is off, then CSS is extracted.

aried3r · 2021-06-09T08:44:41Z

However, JS devs would expect that HMR is on by default, as that's the default for creating-react-app, gatsby, and next.js.

I agree with this statement. Having worked mostly with Rails and some Next.js recently, there is a definitive gap between the default webpacker experience and the default experience of the mentioned frameworks.

But I also understand the concern that onboarding people coming from the assets pipeline is important.

justin808 · 2021-06-09T09:51:36Z

@rossta @dhh I changed the PR to address @rossta comments.

justin808 · 2021-06-09T09:56:02Z

lib/webpacker/helper.rb

@@ -137,6 +141,9 @@ def preload_pack_asset(name, **options)
  #   <%= stylesheet_pack_tag 'calendar' %>
  #   <%= stylesheet_pack_tag 'map' %>
  def stylesheet_pack_tag(*names, **options)
+    css_inline = Webpacker.dev_server.hmr? && Webpacker.dev_server.configured_and_running?
+    return "" if css_inline
+


@dhh and @rossta, does the above make it clear when to not have the style tag if we want CSS inlined?

I would pull this up into Webpacker, so it's Webpacker.inlining_css?.

Done. How's this look?

justin808 · 2021-06-09T10:00:17Z

package/environments/development.js

  if (devServer.hmr) {
    devConfig = merge(devConfig, {
-      output: { filename: '[name]-[hash].js' },
-      plugins: [new webpack.HotModuleReplacementPlugin()]


Previously, we always included the HotModuleReplacementPlugin even if HMR was off. So it's no change if the command line option --hot is always added.

guillaumebriday · 2021-06-14T22:06:54Z

package/utils/helpers.js

+const runningWebpackDevServer = process.env.WEBPACK_DEV_SERVER &&
+  process.env.WEBPACK_DEV_SERVER !== 'undefined'


Is this a string? And not the undefined keywork? 🤔

> process.env.WEBPACK_DEV_SERVER undefined > process.env.WEBPACK_DEV_SERVER !== 'undefined' true > process.env.WEBPACK_DEV_SERVER !== undefined false >

Suggested change

const runningWebpackDevServer = process.env.WEBPACK_DEV_SERVER &&

process.env.WEBPACK_DEV_SERVER !== 'undefined'

// When running the webpack-dev-server, this ENV value is set.

const runningWebpackDevServer = process.env.WEBPACK_DEV_SERVER !== undefined

Updated to check against 'true'.

justin808 · 2021-06-21T10:33:53Z

@guillaumebriday Almost ready. Just got to fix the jest tests.

1. Use style-loader for CSS modules HMR 2. Don't extract CSS if running the webpack-dev-server

Webpacker.dev_server.running? might appear to be costly, but it first checks that the dev_server value is on the config Hash. Renaming the method makes it clear that the first check is whether it is configured. Since the dev_server would ever be configured on production, there is no cost for calling this method on production.

* HMR default to true for webpacker.yml * inline, injectClient, and injectHot default to the value of hmr * hmr turned on for the webpack dev server if the hmr config is true * using command line option for --hot as the docs recommend that over the plugin.

1. hmr default is false 2. CSS is extracted unless yml file configuredd HMR and running webpack-dev-server

justin808 · 2021-06-23T09:46:05Z

lib/webpacker/helper.rb

@@ -137,6 +141,9 @@ def preload_pack_asset(name, **options)
  #   <%= stylesheet_pack_tag 'calendar' %>
  #   <%= stylesheet_pack_tag 'map' %>
  def stylesheet_pack_tag(*names, **options)
+    css_inline = Webpacker.dev_server.hmr? && Webpacker.dev_server.configured_and_running?
+    return "" if css_inline
+


Done. How's this look?

justin808 · 2021-06-23T09:47:09Z

package/env.js

 module.exports = {
  railsEnv: railsEnv && railsEnv.match(regex) ? railsEnv : DEFAULT,
  nodeEnv,
  isProduction,
-  isDevelopment
+  isDevelopment,
+  runningWebpackDevServer


runningWebpackDevServer feels like an env value.

justin808 · 2021-06-23T09:49:06Z

@guillaumebriday @dhh this PR now has all the requested changes.

iguanus · 2021-06-24T00:53:53Z

docs/developing_webpacker.md

+
+## Update the Package Code
+1. Make some JS change in WEBPACKER_DIR
+2. Run `yalc push` and your changes will to your `TEST_APP_DIR`'s node_modules.


I think we missed a verb in this phrase.

... and your changes will be pushed to your...

Fixed! Thanks!

iguanus · 2021-06-24T01:52:24Z

Looks solid to me. Kudos on the helper variables naming!

tommy-gilligan · 2021-06-25T09:07:51Z

responding to #3055 (comment)

Followed the instructions to test:

Checkout branch justin808/change-css-loader
bundle && yarn
(had to bundle update webpacker first because fatal: Could not parse object '1a680abb22154b97b6e86ced38b15cfa23a441da'. )
Run foreman start -f Procfile.dev-hmr
Foreman output
Open up http://localhost:3000/hello_world and keep it visible to see the hot reloading
at this point i think i should see something that is green but i don't

aborted testing here

Edit the string on line 17 of HelloWorld.tsx and see the string change in the browser change upon saving the file.
Edit the text color on line 2 of HelloWorld.module.css and see the browser change upon saving the file.

i tried with
node v14.15.4
and again with a fresh install of node v16.4.0

justin808 · 2021-06-26T03:44:59Z

Hi @tomgilligan, please pul the latest code from shakacode/react_on_rails_demo_ssr_hmr#12

I had updated rails/webpacker, but I forgot to push my test code!

@guillaumebriday Can you give it a try?

tommy-gilligan · 2021-06-26T12:16:56Z

Thank you for the update @justin808 . I now get this for both node v14.15.4 and v16.4.0 .

22:15:40 wp-client.1 | <s> [webpack.Progress] 9% setup compilation DefinePlugin
22:11:25 wp-client.1 | [webpack-cli] TypeError: Cannot read property 'get' of undefined
22:11:25 wp-client.1 |     at exports.provide (/Users/tomgilligan/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/node_modules/@rails/webpacker/node_modules/webpack/lib/util/MapHelpers.js:17:20)
22:11:25 wp-client.1 |     at /Users/tomgilligan/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/node_modules/@rails/webpacker/node_modules/webpack/lib/DefinePlugin.js:290:6
22:11:25 wp-client.1 |     at Hook.eval [as call] (eval at create (/Users/tomgilligan/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/node_modules/tapable/lib/HookCodeFactory.js:19:10), <anonymous>:300:1)
22:11:25 wp-client.1 |     at Hook.CALL_DELEGATE [as _call] (/Users/tomgilligan/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/node_modules/tapable/lib/Hook.js:14:14)
22:11:25 wp-client.1 |     at Compiler.newCompilation (/Users/tomgilligan/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/node_modules/webpack/lib/Compiler.js:988:26)
22:11:25 wp-client.1 |     at /Users/tomgilligan/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/node_modules/webpack/lib/Compiler.js:1029:29
22:11:25 wp-client.1 |     at Hook.eval [as callAsync] (eval at create (/Users/tomgilligan/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:62:1)
22:11:25 wp-client.1 |     at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (/Users/tomgilligan/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/node_modules/tapable/lib/Hook.js:18:14)
22:11:25 wp-client.1 |     at Compiler.compile (/Users/tomgilligan/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/node_modules/webpack/lib/Compiler.js:1024:28)
22:11:25 wp-client.1 |     at /Users/tomgilligan/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh/node_modules/webpack/lib/Watching.js:112:19
22:11:25 wp-client.1 | exited with code 2

justin808 · 2021-06-26T21:48:56Z

@tomgilligan Please pull the latest from my branch. Updating dependencies fixed the issue.

justin808 · 2021-06-26T22:48:06Z

I made a video showing this working: https://youtu.be/lIkhnLXIH6I

jcohenho · 2021-06-28T20:00:24Z

LGTM! Any idea when this will get merged?

vtamara · 2021-06-29T00:34:53Z

I was able to run the proposed test on the following platform:

OS: OpenBSD/adJ 6.9
Ruby: 3.0.1
Node: 12.16.1

I had to do the following changes:

In the Gemfile remove ruby 2.7.3 and replace mini_racer with execjs
In config/webpacker.yml adding poll: true under watch_options:
In package.json adding "style-loader": "^3.0.0"

Thank you for the good job.

Update to include: rails/webpacker#3031 Check if inlining CSS before doing hot refresh We can only use the ReactRefreshWebpackPlugin if the server is running and doing HMR, which is what the inliningCss does.

PR #3031 added a dependency for `style-loader` when CSS is inlined here: https://github.com/rails/webpacker/blob/master/package/utils/get_style_rule.js#L16 The integration instructions for supporting CSS does not include that package for the `yarn add` so no one would know to include it until they try to run the dev server locally and are greeted with a missing package error. ``` ERROR in ./app/packs/entrypoints/application.js 8:0-33 Module not found: Error: Can't resolve 'style-loader' in '/path/to/webapp' resolve 'style-loader' in '/path/to/webapp' ``` Adding the package with `yarn add` fixes the issue. I added `style-loader` to the README under the CSS integration section.

justin808 force-pushed the justin808/change-css-loader branch from a8df863 to 284204e Compare May 26, 2021 02:58

This was referenced May 26, 2021

Webpacker v6 -- Blocker to GA -- extract_css removal #2838

Closed

Enable CSS Modules and Hot Reloading shakacode/react_on_rails_demo_ssr_hmr#12

Merged

How stable is v6.0.0.beta.7? #2999

Closed

dhh reviewed Jun 2, 2021

View reviewed changes

justin808 force-pushed the justin808/change-css-loader branch from 8e2f3c7 to 8a8a90f Compare June 3, 2021 09:03

justin808 commented Jun 3, 2021

View reviewed changes

justin808 force-pushed the justin808/change-css-loader branch 2 times, most recently from 3f1adf0 to 90e80d5 Compare June 3, 2021 11:15

justin808 force-pushed the justin808/change-css-loader branch from be46fe9 to a219f64 Compare June 9, 2021 09:48

justin808 force-pushed the justin808/change-css-loader branch from a219f64 to 8fbc86f Compare June 9, 2021 09:55

justin808 commented Jun 9, 2021

View reviewed changes

justin808 force-pushed the justin808/change-css-loader branch from 8fbc86f to 6ca0bc0 Compare June 9, 2021 20:01

guillaumebriday reviewed Jun 14, 2021

View reviewed changes

justin808 added 7 commits June 21, 2021 00:35

CSS Modules HMR working again

37f487c

1. Use style-loader for CSS modules HMR 2. Don't extract CSS if running the webpack-dev-server

Fix jest and ruby tests

1992dd3

Address review

b68964f

1. hmr default is false 2. CSS is extracted unless yml file configuredd HMR and running webpack-dev-server

Address review comments

3a44839

Refactor to allow exporting logic of inliningCss

9e282f6

justin808 force-pushed the justin808/change-css-loader branch from ce8929e to 9e282f6 Compare June 21, 2021 10:35

justin808 force-pushed the justin808/change-css-loader branch 2 times, most recently from 65d4f71 to 0e16135 Compare June 23, 2021 09:16

Fix jest and Ruby tests

ab5a7a5

justin808 force-pushed the justin808/change-css-loader branch from 0e16135 to ab5a7a5 Compare June 23, 2021 09:39

justin808 commented Jun 23, 2021

View reviewed changes

iguanus reviewed Jun 24, 2021

View reviewed changes

Update developing_webpacker.md

2bb3441

justin808 mentioned this pull request Jun 24, 2021

Process towards release (or "what's blocking Webpacker 6")? #3055

Closed

Update changelog

605ade6

dhh merged commit 032c2d1 into rails:master Jun 28, 2021

justin808 deleted the justin808/change-css-loader branch June 29, 2021 04:04

This was referenced Jul 15, 2021

Skip contenthash when not production #3060

Merged

Include style-loader for CSS integration #3092

Merged

justin808 mentioned this pull request Aug 26, 2021

Have extract_css be smart about running the webpack_dev_server #2711

Closed

aried3r mentioned this pull request Oct 19, 2021

Watch rails views and reload page on changes #1879

Closed

		const runningWebpackDevServer = process.env.WEBPACK_DEV_SERVER &&
		process.env.WEBPACK_DEV_SERVER !== 'undefined'

CSS Modules HMR working again #3031

CSS Modules HMR working again #3031

Conversation

justin808 commented May 26, 2021 • edited Loading

Changes

To Test

justin808 commented Jun 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin808 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossta commented Jun 3, 2021 • edited Loading

justin808 commented Jun 4, 2021 • edited Loading

justin808 commented Jun 9, 2021

aried3r commented Jun 9, 2021

justin808 commented Jun 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guillaumebriday Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin808 commented Jun 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin808 commented Jun 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iguanus commented Jun 24, 2021

tommy-gilligan commented Jun 25, 2021 • edited Loading

aborted testing here

justin808 commented Jun 26, 2021

tommy-gilligan commented Jun 26, 2021

justin808 commented Jun 26, 2021

justin808 commented Jun 26, 2021

jcohenho commented Jun 28, 2021 • edited Loading

vtamara commented Jun 29, 2021

justin808 commented May 26, 2021 •

edited

Loading

rossta commented Jun 3, 2021 •

edited

Loading

justin808 commented Jun 4, 2021 •

edited

Loading

guillaumebriday Jun 14, 2021 •

edited

Loading

tommy-gilligan commented Jun 25, 2021 •

edited

Loading

jcohenho commented Jun 28, 2021 •

edited

Loading