Add support for disabling style-loader via inline_css: false.

[cheald]

This PR is for #68

This will cause style-loader to not be used if inline_css: false is set in the dev_server options. Any value other than false (including nil) will result in the current behavior.

I have a test for the inlining_css? method, and I did manually test that this has the desired effect in my own application, but I'm not sure how to best wire up a test for it otherwise.

[tomdracz]

@cheald Should inline_styles be incline_css here?

[cheald]

Yes, you're right. Good catch!

[tomdracz]

Two things @cheald @justin808

• We should add new config value to https://github.com/shakacode/shakapacker/blob/master/lib/install/config/webpacker.yml, ideally matching the current behaviour (i.e. inlining styles enabled wherever we've got HMR and dev server)
• Just a thought - if mini-css-extract-plugin does HMR now, do we still need style-loader? What's the benefit of one over the other? If we're gearing up for next major version, that could be the time to remove dependencies. Would be great to understand pros cons of both approaches of loading styles in dev mode

[justin808]

@cheald I agree with @tomdracz's feedback.

[cheald]

style-loader is still how people are "used" to CSS HMR in Webpack. mini-css-extract-plugin's HMR functionality a) reloads the existing CSS and b) patches in the changed CSS on top of it, which is more data transferred and may cause a brief FOUC.

In both cases, Webpack's compilation time doesn't seem to change (as one would expect, since the dirty files don't change, and neither style-loader or mini-css-extract-plugin are particularly heavy).

style-loader

benefits

• The well-trod path, people are used to it
• No FOUC on HMR refreshes
• Smaller/faster incremental updates.

drawbacks

• Inflated JS deliverable size; requires JS execution before CSS is available
• FOUC on initial page load
• Adds an extra dependency
• Divergence in delivery mechanism from production

mini-extract-css-plugin:

Benefits

• Required for production, so it's going to be in play anyhow. Using only it simplifies the config and eliminates the style-loader dependency.
• No FOUC on initial page loads
• CSS delivered via <link> tags matches the mechanism used in production (I have been guilty of omitting my stylesheet_pack_tag for my first deploy because CSS worked fine with just the javascript_pack_tag in development.)

Drawbacks

• Potential for FOUC on HMR refreshes
• More data transferred per refresh (full stylesheet reload, rather than just an incremental patch). Not likely to be noticed for local development, but still a technical difference. (Edit: This may only be the case when you're using local CSS modules, as I am)

I'd likely opt to just use mini-extract-css-plugin all the time, but I can see why others may want to retain the style-loader path.

[tomdracz]

Thanks @cheald Good writeup! Makes sense for this to be opt-in behaviour then.

Might be something that could be recommended approach though, I also hit issues of missing stylesheet_pack_tag and not realising because style loader was getting everything through fine. Getting the dev and production envs closer together without that surprise is a win in my book!

cc @justin808

[tomdracz]

@cheald LGTM - can you add a changelog entry also under unreleased? https://github.com/shakacode/shakapacker/blob/master/CHANGELOG.md#unreleased

[cheald]

Done and done.

[justin808]

What is FOUC?

@cheald can you take your comments above on the tradeoffs and put them into a file in the docs directory.

@tomdracz how about /docs/hmr.md?

[tomdracz]

What is FOUC?

@cheald can you take your comments above on the tradeoffs and put them into a file in the docs directory.

@tomdracz how about /docs/hmr.md?

FOUC = flash of unstyled content @justin808 :)

Docs at docs/hmr.md make sense to me. We can then append to it and run through HMR configs for the other bits that are needed here. If you put the comparison in new file there @cheald I'm happy to add more bits to it

[justin808]

@tomdracz @cheald is this ready for merge? Or wait for the doc changes?

[tomdracz]

@justin808 let's merge and I'll create a follow up with doc updates 👍

[cheald]

Hey guys, sorry about the delay on this. I've been out of pocket this week. I'm happy to submit another PR for the documentation if that's still needed.

[justin808]

@cheald YES! Please do! I want to get that out, and then I'll make the release at the same time.

[justin808]

@cheald and @tomdracz I'd like to push out a release, and I'd like to get the docs updated for that to happen.