Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悶 CSS won't hot-reload on SSR'ed page #106

Closed
ivan-aksamentov opened this issue Sep 24, 2018 · 7 comments
Closed

馃悶 CSS won't hot-reload on SSR'ed page #106

ivan-aksamentov opened this issue Sep 24, 2018 · 7 comments

Comments

@ivan-aksamentov
Copy link

ivan-aksamentov commented Sep 24, 2018

On first loaded (server-rendered) page CSS is not hot reloading, despite a report in browser console. It does not matter which page you are server-rendering. JS hot reloads fine on any page. CSS hot reloads fine on client-rendered pages too.

Repro:

Any commit will do in this repo, here is the last one:

https://github.com/ivan-aksamentov/reactlandia-bolerplate-lite/tree/781f3d0618770016c26b3a25ef23e746700e41d8

Quick start on Unix:

  • install and run
    git clone https://github.com/ivan-aksamentov/reactlandia-bolerplate-lite.git
    cd reactlandia-bolerplate-lite
    git checkout 781f3d0618770016c26b3a25ef23e746700e41d8
    npm install
    npm run dev

Scenario 1: "CSS on SSRed page is not hot reloaded" :

  • open localhost:3000 in browser, "Home" page is loaded which is server-rendered
  • open dev tools console in browser
  • open src/pages/Home.css and change the background-color.
  • a warning and hot reload is reported in dev tools console
  • no actual color change happen

Scenario 2: "CSS on client-rendered page is hot reloaded" :

  • reload the "Home" page (/) in browser, without cache (Ctrl+F5)
  • Click "About" to navigate to a client-rendered page
  • make changes to src/pages/About.css
  • changes are hot reloaded succesfully

Scenario 3: "CSS on SSRed page is not hot reloaded on any page, not just Home":

  • reload the "About" page (/about) in browser, without cache (Ctrl+F5), to obtain it's server-rendered copy this time
  • Perform same steps
  • No hot reloading of CSS

Scenarios 5 and 6: "JS is hot reloaded just fine on both, server- and client-rendered pages"

  • Do the same steps, but change JS files rather than CSS

Source files of interest:

Ubuntu 16.04
node 10.11
npm 6.4.1
"@babel/core": "^7.1.0",
"webpack": "^4.19.1",
"react": "^16.5.2",
"react-universal-component": "^4.0.0-alpha.0",
"babel-plugin-universal-import": "^4.0.0-alpha.3",
"extract-css-chunks-webpack-plugin": "^3.1.2-beta.6",
"webpack-flush-chunks": "^3.0.0-alpha.1"
"redux-first-router": "^1.9.19",
"redux": "^4.0.0",

What I tried:

  • No combination of any of options {cssModules, reloadAll, hot} of extract-css-chunks-webpack-plugin helps:
    new ExtractCssChunks({
      filename: IS_DEVELOPMENT ? '[name].css' : '[name].[chunkhash:5].css',
      chunkFilename: IS_DEVELOPMENT ? '[name].css' : '[name].[chunkhash:5].css',
      cssModules: USE_CSS_MODULES,
      reloadAll: IS_DEVELOPMENT,
      hot: IS_DEVELOPMENT,
    }),

Workaround:

For comfortable dev experience I keep applying a monkey patch that deletes the if statement, so that else branch (full hot reload) is always taken

TODO

Look at DOM and see what hot-reload chunks are being added and in what order and how it differs in SSRed and client-rendered pages. It seems like it may be a CSS ordering issue.

@ScriptedAlchemy
Copy link
Collaborator

Wow 馃槸 thanks for the detail!

I鈥檒l investigate this today or tomorrow 鉂わ笍鉂わ笍馃挴

@ivan-aksamentov
Copy link
Author

ivan-aksamentov commented Sep 24, 2018

@ScriptedAlchemy Thanks Zack.
The easiest one-line fix would be to change that if statement in hotModuleReplacement.js, so that reloadAll takes precedence and shuts everyone up with reloadAll():

    const reloaded = reloadStyle(src);
-    if (reloaded && !options.reloadAll) {
+    if (!options.reloadAll || reloaded) {
      console.log('[HMR] css reload %s', src.join(' '));
    } else {
      console.log('[HMR] Reload all css');
      reloadAll();
    }

That could be an easy pull request for me, but I think that might just hide the real issue somewhere deep inside, that would explode into our faces later on. If it's dev-only we can probably take a risk, but if it has consequences for production builds, I'd prefer some more careful consideration :)

@ScriptedAlchemy
Copy link
Collaborator

Go ahead and open a PR!!

HMR is all in dev anyways. Worst thing that may happen is something reloads more than required.

I鈥檇 merge it into my branch and release another beta. Give it a few days and see if anyone complains.
I don鈥檛 see any serious issues. Just check of HMR is firing excessively, if not - then I鈥檒l put it out as a beta.

I think your issues were the last ones before rolling an update

@ivan-aksamentov
Copy link
Author

ivan-aksamentov commented Sep 25, 2018

@ScriptedAlchemy Okay, upon further examination it turned out to be not so easy :).
The thing is that options.reloadAll is undefined and !options.reloadAll always evaluates truly.
I think that options object is not being passed through properly and some of the fields are being lost somewhere along the way from webpack config file to hotModuleReplacement.js.

By the way, is it better to investigate and base my PR on master or on hmr-multi-instance?

@ivan-aksamentov
Copy link
Author

ivan-aksamentov commented Sep 25, 2018

Additionally I noticed that those erroneous "not reloading" reloads try to reload main.css chunk instead of home.css (when I modify Home.css) on SSRed page. On client-rendered page they reload the correct chunk. Things are getting more and more exciting!

@ScriptedAlchemy
Copy link
Collaborator

it would be best to PR multi hmr, thats the beta branch currently

ivan-aksamentov added a commit to ivan-aksamentov/extract-css-chunks-webpack-plugin that referenced this issue Sep 28, 2018
`options.reloadAll` had no effect, because they were incorrectly copied into the webpack's loader.options and could never reach `hotModuleReplacement.js`. This commit fixes the copy site, allowing for options to pass through. Fixes faceyspacey#106
@ivan-aksamentov
Copy link
Author

PR #107 should fix options.reloadAll being undefined, as well as potential problems with other options. With reloadAll: true passed to webpack plugin, for example like that:

{
    new ExtractCssChunks({
      filename: IS_DEVELOPMENT ? '[name].css' : '[name].[chunkhash:5].css',
      chunkFilename: IS_DEVELOPMENT ? '[name].css' : '[name].[chunkhash:5].css',
      cssModules: false,
      reloadAll: true,
      hot: true,
    }),
}

hot reloader will now reload all CSS perfectly at all times.

However, setting this option just hides the fact that const reloaded = reloadStyle(src); is failing to reload CSS on SSRed page, while shamelessly returning reloaded == true. More investigation/PRs are welcome! :)

ScriptedAlchemy pushed a commit that referenced this issue Oct 1, 2018
`options.reloadAll` had no effect, because they were incorrectly copied into the webpack's loader.options and could never reach `hotModuleReplacement.js`. This commit fixes the copy site, allowing for options to pass through. Fixes #106
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants