aurelia / loader-webpack

An implementation of Aurelia's loader interface to enable webpack.
MIT License
26 stars 9 forks source link

Injected style elements: Multiple /* sourceURL */ lines causes browser devtools issues #61

Closed josundt closed 2 years ago

josundt commented 2 years ago

I'm submitting a bug report

Please tell us about your environment:

Current behavior: For some time our Aurelia application had issues with altering CSS properties in chromium browsers (Chrome/Edge) devtools. When trying to toggle certain CSS properties on/off through the "Styles" tab of the devtools "Elements" inspector, the CSS layout for major parts of the app immediately gets corrupted; certain selectors suddenly appear multiple times in the "Styles" tab, and the whole page looks messed up.

This really has complicated working with our app's design, so today I set out to find the root cause of the issue. TLDR: I found that a fix is required in the aurelia-loader-webpack package, I have therfore prepared a PR.

We are using Sass (.scss) source code, and we use a standard au-cli generaated webpack configuration pipeline: sass-loader => postcss-loader => css-loader for files <require>d from HTML templates, and with the additioinal => style-loader for files imported from ES modules. (Source maps are enabled for the loaders when webpack mode=development).

Steps taken to identify the problem:

It looks like multiple sourceURL lines affect the way sourceMapping works in chromium devtools; it seems like the devtools only considers the last of the sourceURL lines.

I therefore have tried to find a way to make aurelia-loader-webpack style element injection work like style-loader; to not include the sourceURL lines, only include the sourceMappingURL line (when sourceMaps).

The .toString() method from the loaded css-loader module that is called in aurelia-loader-webpack.ts:241 returns the full CSS content, with both sourceURL and sourceMappingURL lines.

I therefore found two possible solutions to fix the problem:

  1. Still use the toString() method, and use a RegExp to remove the sourceURL lines (only executed when the loaded css-loader module has sourceMaps).
  2. Manually construct the CSS content with potential sourceMappingURL from the loaded css-loader module properties. This can be accomplished by reusing some of the code from the css-loader repository, and skip the sourceURL generation.

Pros/cons: Solution 1: Low vulnerability to breaking changes in css-loader package but has a minor performance impact. Solution 2: More vulnerable to major changes in the css-loader package, but with zero performance impact.

I have forked this repository and implemented solutions 1. and 2. on two different branches:

I have tested both solutions in our app, and they both fix the problem. PS! Solution 2 reuses the exact code from the toString() in the style-loader loader, but the writing of the sourceURL lines have been excluded.

I hope you acknowledge this as a problem, and realize that a fix is needed here. I have not created a PR yet, because I wanted some feedback on which of the solutions you prefer. Please get back to me, hopefully with your preferred solution, so that I can make a PR.

Expected/desired behavior: Altering CSS through devtools should work.

josundt commented 2 years ago

I have not received any feedback on this. This is a low-risk simple change that mitigates the problem in chromium devtools for CSS source maps, and I have tested that it fixes the problem, and that everything incl. CSS source maps works as expected with the fix.

May I propose that I make a PR for solution 2 above?

@bigopon I see that you made the last commits to this repo, and since you have approved various Aurelia 1 PRs from me earlier; any chance you could have a quick look and make a decision?

bigopon commented 2 years ago

@josundt thanks for the detailed analysis. I saw this issue earlier, and set out a time to read it properly and reply then managed to forget 😓 . That aside, if both solutions work then I'd prefer 1, since it's less prone to breakage from css changes, if I understand /assume the code behavior correctly. For performance, I think it won't matter that much since the module won't be repeatedly load.

josundt commented 2 years ago

@bigopon Perfect. I'll create the PR now.