feat: add option to warn at runtime when non-existent properties are accessed

[DrewML]

What kind of change does this PR introduce?
A new query param is added to the loader (cssModuleDevWarnings) that can be used in conjunction with CSS modules. At runtime, this will emit a console.warn anytime a non-existent class name is accessed on the CSS module.

Did you add tests for your changes?
Not yet. Happy to add them, just wanted to check if you'd be willing to accept this type of change before investing the time to write tests.

If relevant, did you update the README?
Yes

Summary

It's been too easy to mistype a property name when assigning a className property in React. At runtime, if className has been assigned to undefined, the element in DevTools will just show as having no classname at all. This is a helpful hint to warn you in development when you've typoed, similar to the types of dev warnings that React gives you.

Does this PR introduce a breaking change?

Should not

Other information
This feature requires that the user have ES2015 proxy support in their browser, so it's super important this never makes it into a user's production code (also because proxies are slow). Looking for any input on how we can make this more obvious besides just docs (maybe a scarier query param name?)

Example

[codecov]

Codecov Report

Merging #421 into master will decrease coverage by -0.32%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #421      +/-   ##
==========================================
- Coverage   98.35%   98.03%   -0.32%     
==========================================
  Files           9        9              
  Lines         304      306       +2     
  Branches       67       68       +1     
==========================================
+ Hits          299      300       +1     
- Misses          5        6       +1

Impacted Files	Coverage Δ
lib/loader.js	98.64% <66.66%> (-1.36%)	❌

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 199897f...75502dd. Read the comment docs.

[codecov]

Codecov Report

Merging #421 into master will decrease coverage by -0.32%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #421      +/-   ##
==========================================
- Coverage   98.35%   98.03%   -0.32%     
==========================================
  Files           9        9              
  Lines         304      306       +2     
  Branches       67       68       +1     
==========================================
+ Hits          299      300       +1     
- Misses          5        6       +1

Impacted Files	Coverage Δ
lib/loader.js	98.64% <66.66%> (-1.36%)	❌

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 199897f...75502dd. Read the comment docs.

[bebraw]

It would be possible to skip cssModuleDevWarnings by checking process.env.NODE_ENV. It's a more implicit solution, though, but people rely on the convention (standard by now). Maybe a direction worth considering.

[michael-ciniawsky]

The overhead and the use of ES2015 Proxies is a personal concern of mine regarding this, how often is it the case that this happens? Could empty modules be filtered instead ?

[bebraw]

@michael-ciniawsky Maybe this is another issue that's better tackled after CSS Modules have been split to a loader of its own?

[michael-ciniawsky]

@bebraw Split 🙃 ? We could do that, maybe the right thing to do, but I could be 'difficult' to do so, I'm still struggling to add it to postcss-loader in a plugable way. But for sure it must be a opt-in since modues are expensive (expecially compose), we could handle it in the loader logic but I'm by no means against extracting it. Like I mentioned before with a few small changes to the postcss-modules-* plugins we could easily add them as a 'normalpostcss plugin +postcss-loader`

[bebraw]

@michael-ciniawsky Figuring out the right composition is the key. Now css-loader does too much. If it can go through postcss-loader and we can improve the pipeline while at it, that's great. A separate loader would be too much if it can be plugged in there.

[michael-ciniawsky]

The problem with seperation is the current tandem of style-loader || ETWP/css-loader interop, if we remove modules from css-loader either postcss-loader or cssm-loader need to work standalone and directly export the result to style-loader/ETWP (cssBase.js) [[css.id, css.mediaQuery, css.content, css.locals]] {Array}

[michael-ciniawsky]

I didn't find the right compromise yet 😛 , since otherwise css-loader again needs to accept two different input variants then and still needs to do the exporting

[bebraw]

@michael-ciniawsky Maybe it's better to try to arrange a little design session at Slack so we have a clear high level idea? One way, forget everything you know about the existing loaders etc. and imagine an ideal pipeline. Then we can see how close to that we can get. 😄

[DrewML]

The overhead and the use of ES2015 Proxies is a personal concern of mine regarding this, how often is it the case that this happens? Could empty modules be filtered instead ?

FWIW, React currently uses Proxy in development mode for similar types of safety checks. Proxies are slow, but not "bring your app to a crawl" slow.

[michael-ciniawsky]

@DrewML Its's more about browser (IE) support then speed

[DrewML]

Its's more about browser (IE) support then speed

We can add in feature detection if that eases your concern at all. Just bail out if no window.Proxy

[michael-ciniawsky]

yeah right we could do that.. 😛 Please be patient for at least 1-2 days since as you may noticed from the discussion started in this thread, css-loader will get a general refactor and as soon as I know more about the concrete how I can guarantee your PR doesn't get stalled meanwhile or what eventual changes are needed here in particular...

[joshwiens]

Given we are splitting modules out of css-loader, should this not also target the new modules repo?:

[michael-ciniawsky]

Yes if we split it up, but the current headache is still the how :), so I left it for now. I need to check the possibilties first and get a working prototype of a splitted approach

[TrySound]

@michael-ciniawsky Named exports has the same target but in build time, so maybe this is not necessary.

[michael-ciniawsky]

@DrewML Ok to close this for now and join discussion around some general loader overhaul in #542 && the develop || new-loader branch instead ? Current 'master' is bugfix now :)

[DrewML]

@michael-ciniawsky Sounds good!