CSS Modules HMR working again (#3031)

* CSS Modules HMR working again

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

* Clarify dev_server.running? first checks configured

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.

* Simplifications and fixes for HMR

* 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.

* Fix jest and ruby tests

* Address review

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

* Address review comments

* Refactor to allow exporting logic of inliningCss

* Fix jest and Ruby tests

* Update developing_webpacker.md

* Update changelog

CHANGELOG.md

@@ -14,20 +14,16 @@ environment.loaders.append('nodeModules', nodeModules)
 ```
 
 - If you have added `environment.loaders.delete('nodeModules')` to your `environment.js`, this must be removed or you will receive an error (`Item nodeModules not found`).
-- The install task will now set the `extract_css` default to `true` in all environments and generate a separate `application.css` file for the default `application` pack, as supported by multiple files per entry introduced in 5.0.0. [#2608](https://github.com/rails/webpacker/pull/2608)
+- `extract_css` option was removed. Webpacker will generate a separate `application.css` file for the default `application` pack, as supported by multiple files per entry introduced in 5.0.0. [#2608](https://github.com/rails/webpacker/pull/2608). However, CSS will be inlined when the webpack-dev-server is used with `hmr: true`. JS package exports `inliningCss`. This is useful to enable HMR for React. 
 - Webpacker's wrapper to the `splitChunks()` API will now default `runtimeChunk: 'single'` which will help prevent potential issues when using multiple entry points per page [#2708](https://github.com/rails/webpacker/pull/2708).
 - Changes `@babel/preset-env` modules option to `'auto'` per recommendation in the Babel docs [#2709](https://github.com/rails/webpacker/pull/2709)
 - Adds experimental Yarn 2 support. Note you must manually set `nodeLinker: node-modules` in your `.yarnrc.yml`.
-
 - Fixes dev server issues [#2898](https://github.com/rails/webpacker/pull/2898)
 
 ### Breaking changes
-
-- Simple webpack config
 - Removed integration installers
 - Splitchunks enabled by default
-- CSS extraction enabled by default
-- Optional CSS support
+- CSS extraction enabled by default, except when devServer is configured and running
 
 ## [[5.4.0]](https://github.com/rails/webpacker/compare/v5.3.0...v5.4.0) - 2021-05-18
 

docs/developing_webpacker.md

@@ -0,0 +1,29 @@
+# Developing Webpacker
+
+It's a little trickier for Rails developers to work on the JS code of a project like rails/webpacker. So here are some tips!
+
+## Use some test app
+For example, for React on Rails Changes, I'm using [shakacode/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh](https://github.com/shakacode/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh).
+This directory is the `TEST_APP_DIR`.
+
+## Fork rails/webpacker
+Let's call the rails/webpacker directory `WEBPACKER_DIR` which has rails/webpacker's `package.json`.
+
+## Changing the Package
+### Setup with Yalc
+Use [`yalc`](https://github.com/wclr/yalc) unless you like yak shaving weird errors. 
+1. In `WEBPACKER_DIR`, run `yalc publish` 
+2. In `TEST_APP_DIR`, run `yarn link @rails/webpacker`
+
+## Update the Package Code
+1. Make some JS change in WEBPACKER_DIR
+2. Run `yalc push` and your changes will be pushed to your `TEST_APP_DIR`'s node_modules.
+3. You may need to run `yarn` in `TEST_APP_DIR` if you added or removed dependencies of rails/webpacker.
+
+## Updating the Ruby Code
+
+For the Ruby part, just change the gem reference `TEST_APP_DIR`, like:
+
+```ruby
+gem "webpacker", path: "../../forks/webpacker"
+```

lib/install/config/webpacker.yml

@@ -25,12 +25,8 @@ development:
     host: localhost
     port: 3035
     public: localhost:3035
-    # Inject browserside javascript that is required by both HMR and Live (full) reload
-    inject_client: true
     # Hot Module Replacement updates modules while the application is running without a full reload
     hmr: false
-    # Inline should be set to true if using HMR; it inserts a script to take care of live reloading
-    inline: true
     # Should we show a full-screen overlay in the browser when there are compiler errors or warnings?
     overlay: true
     # Should we use gzip compression?
@@ -42,7 +38,7 @@ development:
     # When enabled, nothing except the initial startup information will be written to the console.
     # This also means that errors or warnings from webpack are not visible.
     quiet: false
-    pretty: false
+    pretty: true
     headers:
       'Access-Control-Allow-Origin': '*'
     watch_options:

lib/webpacker.rb

@@ -30,7 +30,7 @@ def ensure_log_goes_to_stdout
     Webpacker.logger = old_logger
   end
 
-  delegate :logger, :logger=, :env, to: :instance
+  delegate :logger, :logger=, :env, :inlining_css?, to: :instance
   delegate :config, :compiler, :manifest, :commands, :dev_server, to: :instance
   delegate :bootstrap, :clean, :clobber, :compile, to: :commands
 end

lib/webpacker/dev_server.rb

@@ -51,12 +51,18 @@ def pretty?
     fetch(:pretty)
   end
 
+  def hmr?
+    fetch(:hmr)
+  end
+
   def env_prefix
     config.dev_server.fetch(:env_prefix, DEFAULT_ENV_PREFIX)
   end
 
   private
     def fetch(key)
+      return nil unless config.dev_server.present?
+
       ENV["#{env_prefix}_#{key.upcase}"] || config.dev_server.fetch(key, defaults[key])
     end
 

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?
 
       rescue Errno::ENOENT, NoMethodError
         $stdout.puts "webpack dev_server configuration not found in #{@config.config_path}[#{ENV["RAILS_ENV"]}]."
@@ -73,12 +74,14 @@ def execute_cmd
         end
 
         if @argv.include?("--debug-webpacker")
-          cmd = [ "node", "--inspect-brk"] + cmd
+          cmd = [ "node", "--inspect-brk", "--trace-warnings" ] + cmd
           @argv.delete "--debug-webpacker"
         end
 
         cmd += ["--config", @webpack_config]
         cmd += ["--progress", "--color"] if @pretty
+
+        cmd += ["--hot"] if @hot
         cmd += @argv
 
         Dir.chdir(@app_path) do

lib/webpacker/helper.rb

@@ -128,6 +128,10 @@ def preload_pack_asset(name, **options)
   #   <link rel="stylesheet" media="screen" href="/packs/calendar-8c7ce31a.chunk.css" />
   #   <link rel="stylesheet" media="screen" href="/packs/map-8c7ce31a.chunk.css" />
   #
+  #   When using the webpack-dev-server, CSS is inlined so HMR can be turned on for CSS,
+  #   including CSS modules
+  #   <%= stylesheet_pack_tag 'calendar', 'map' %> # => nil
+  #
   # DO:
   #
   #   <%= stylesheet_pack_tag 'calendar', 'map' %>
@@ -137,6 +141,8 @@ def preload_pack_asset(name, **options)
   #   <%= stylesheet_pack_tag 'calendar' %>
   #   <%= stylesheet_pack_tag 'map' %>
   def stylesheet_pack_tag(*names, **options)
+    return "" if Webpacker.inlining_css?
+
     stylesheet_link_tag(*sources_from_manifest_entrypoints(names, type: :stylesheet), **options)
   end
 

lib/webpacker/instance.rb

@@ -34,4 +34,8 @@ def manifest
   def commands
     @commands ||= Webpacker::Commands.new self
   end
+
+  def inlining_css?
+    dev_server.hmr? && dev_server.running?
+  end
 end

lib/webpacker/railtie.rb

@@ -8,8 +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
-    if insert_middleware
+    if Webpacker.config.dev_server.present?
       app.middleware.insert_before 0,
         Rails::VERSION::MAJOR >= 5 ?
           Webpacker::DevServerProxy : "Webpacker::DevServerProxy", ssl_verify_none: true

package/__tests__/development.js

@@ -14,7 +14,7 @@ describe('Development environment', () => {
     test('should use development config and environment including devServer if WEBPACK_DEV_SERVER', () => {
       process.env.RAILS_ENV = 'development'
       process.env.NODE_ENV = 'development'
-      process.env.WEBPACK_DEV_SERVER = 'YES'
+      process.env.WEBPACK_DEV_SERVER = 'true'
       const { webpackConfig } = require('../index')
 
       expect(webpackConfig.output.path).toEqual(resolve('public', 'packs'))
@@ -23,7 +23,7 @@ describe('Development environment', () => {
         devServer: {
           host: 'localhost',
           port: 3035,
-          injectClient: true
+          injectClient: false
         }
       })
     })

package/__tests__/env.js

@@ -15,7 +15,8 @@ describe('Env', () => {
       railsEnv: 'development',
       nodeEnv: 'development',
       isProduction: false,
-      isDevelopment: true
+      isDevelopment: true,
+      runningWebpackDevServer: false
     })
   })
 
@@ -26,7 +27,8 @@ describe('Env', () => {
       railsEnv: 'development',
       nodeEnv: 'production',
       isProduction: true,
-      isDevelopment: false
+      isDevelopment: false,
+      runningWebpackDevServer: false
     })
   })
 
@@ -37,7 +39,8 @@ describe('Env', () => {
       railsEnv: 'production',
       nodeEnv: 'production',
       isProduction: true,
-      isDevelopment: false
+      isDevelopment: false,
+      runningWebpackDevServer: false
     })
   })
 
@@ -48,7 +51,8 @@ describe('Env', () => {
       railsEnv: 'staging',
       nodeEnv: 'production',
       isProduction: true,
-      isDevelopment: false
+      isDevelopment: false,
+      runningWebpackDevServer: false
     })
   })
 })

package/env.js

@@ -16,9 +16,15 @@ const config = safeLoad(readFileSync(configPath), 'utf8')
 const availableEnvironments = Object.keys(config).join('|')
 const regex = new RegExp(`^(${availableEnvironments})$`, 'g')
 
+// v4 of webpack-dev-server will switch to WEBPACK_DEV_SERVE
+// https://github.com/rails/webpacker/issues/3057
+const runningWebpackDevServer = process.env.WEBPACK_DEV_SERVER === 'true' ||
+  process.env.WEBPACK_DEV_SERVE === 'true'
+
 module.exports = {
   railsEnv: railsEnv && railsEnv.match(regex) ? railsEnv : DEFAULT,
   nodeEnv,
   isProduction,
-  isDevelopment
+  isDevelopment,
+  runningWebpackDevServer
 }

package/environments/development.js

@@ -1,8 +1,8 @@
 const { merge } = require('webpack-merge')
-const webpack = require('webpack')
 
 const baseConfig = require('./base')
 const devServer = require('../dev_server')
+const { runningWebpackDevServer } = require('../env')
 
 const { outputPath: contentBase, publicPath } = require('../config')
 
@@ -11,14 +11,10 @@ let devConfig = {
   devtool: 'cheap-module-source-map'
 }
 
-if (
-  process.env.WEBPACK_DEV_SERVER &&
-  process.env.WEBPACK_DEV_SERVER !== 'undefined'
-) {
+if (runningWebpackDevServer) {
   if (devServer.hmr) {
     devConfig = merge(devConfig, {
-      output: { filename: '[name]-[hash].js' },
-      plugins: [new webpack.HotModuleReplacementPlugin()]
+      output: { filename: '[name]-[hash].js' }
     })
   }
 
@@ -33,8 +29,9 @@ if (
       https: devServer.https,
       hot: devServer.hmr,
       contentBase,
-      inline: devServer.inline,
-      injectClient: devServer.inject_client,
+      inline: devServer.inline || devServer.hmr,
+      injectClient: devServer.hmr,
+      injectHot: devServer.hmr,
       useLocalIp: devServer.use_local_ip,
       public: devServer.public,
       publicPath,

package/index.js

@@ -10,6 +10,7 @@ const config = require('./config')
 const devServer = require('./dev_server')
 const { nodeEnv } = require('./env')
 const { moduleExists, canProcess } = require('./utils/helpers')
+const inliningCss = require('./inliningCss')
 
 const webpackConfig = () => {
   const path = resolve(__dirname, 'environments', `${nodeEnv}.js`)
@@ -25,5 +26,6 @@ module.exports = {
   rules,
   moduleExists,
   canProcess,
+  inliningCss,
   ...webpackMerge
 }

package/inliningCss.js

@@ -0,0 +1,7 @@
+const { runningWebpackDevServer } = require('./env')
+const devServer = require('./dev_server')
+
+// This logic is tied to lib/webpacker/instance.rb
+const inliningCss = runningWebpackDevServer && devServer.hmr
+
+module.exports = inliningCss

package/utils/get_style_rule.js

@@ -1,6 +1,6 @@
 /* eslint global-require: 0 */
-
 const { canProcess, moduleExists } = require('./helpers')
+const inliningCss = require('../inliningCss')
 
 const getStyleRule = (test, preprocessors = []) => {
   if (moduleExists('css-loader')) {
@@ -10,8 +10,10 @@ const getStyleRule = (test, preprocessors = []) => {
         options: { sourceMap: true }
       }))
 
+    // style-loader is required when using css modules with HMR on the webpack-dev-server
+
     const use = [
-      { loader: require('mini-css-extract-plugin').loader },
+      inliningCss ? 'style-loader' : require('mini-css-extract-plugin').loader,
       {
         loader: require.resolve('css-loader'),
         options: {

test/dev_server_runner_test.rb

@@ -40,6 +40,7 @@ def dev_server.host; "localhost"; end
     def dev_server.port; "3035"; end
     def dev_server.pretty?; false; end
     def dev_server.https?; true; end
+    def dev_server.hmr?; false; end
     Webpacker::DevServer.stub(:new, dev_server) do
       verify_command(cmd, argv: ["--https"])
     end

test/test_app/config/webpacker.yml

@@ -43,8 +43,6 @@ development:
     port: 3035
     public: localhost:3035
     hmr: false
-    # Inline should be set to true if using HMR
-    inline: true
     overlay: true
     disable_host_check: true
     use_local_ip: false

test/webpacker_test.rb

@@ -10,4 +10,21 @@ def test_config_params
       assert_equal "test", Webpacker.config.env
     end
   end
+
+  def test_inline_css_no_dev_server
+    assert !Webpacker.inlining_css?
+  end
+
+  def test_inline_css_with_hmr
+    dev_server = Webpacker::DevServer.new({})
+    def dev_server.host; "localhost"; end
+    def dev_server.port; "3035"; end
+    def dev_server.pretty?; false; end
+    def dev_server.https?; true; end
+    def dev_server.hmr?; true; end
+    def dev_server.running?; true; end
+    Webpacker.instance.stub(:dev_server, dev_server) do
+      assert Webpacker.inlining_css?
+    end
+  end
 end