Skip to content
This repository was archived by the owner on Aug 28, 2026. It is now read-only.

fix: remove sourceURL from emitted CSS - #1487

Merged
alexander-akait merged 1 commit into
webpack:masterfrom
alan-agius4:css-source-url
Dec 14, 2022
Merged

alexander-akait merged 1 commit into
webpack:masterfrom
alan-agius4:css-source-url

Conversation

@alan-agius4

Copy link
Copy Markdown
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

In this case the sourceURL is not needed as the entire sourcemap which cotains all the information is being embeeded.

Multiple sourceURL is also causing issues when in Chrome devtools.

Additional Info

See: angular/angular-cli#24414 for more information.

@alexander-akait

Copy link
Copy Markdown
Member

Can you create reproducible example using pure webpack, because I can't get two sourceURL

/*# sourceURL=webpack://./src/app/b/b.component.scss */
/*# sourceURL=webpack://./src/styles.scss */

@alan-agius4

alan-agius4 commented Dec 13, 2022

Copy link
Copy Markdown
Contributor Author

@alexander-akait all you need to do is enable sourcemaps in sass-loader.

Here's an example

master...alan-agius4:css-loader:repro#diff-b15393677601d8b57053e1e25f5e3e9ef695b570273563e83f2217315bf28cf1R2520-R2522

A sourceURL comment will be added for each file in the sass compilation.

In this case the `sourceURL` is not needed as the entire sourcemap which contains all the information is being embedded.

Multiple `sourceURL` is also causing issues when in Chrome devtools. See: angular/angular-cli#24414 for more information.
@codecov

codecov Bot commented Dec 14, 2022

Copy link
Copy Markdown

Codecov Report

Base: 96.82% // Head: 96.81% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (05c3699) compared to base (3f3f302).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1487      +/-   ##
==========================================
- Coverage   96.82%   96.81%   -0.01%     
==========================================
  Files          12       12              
  Lines        1133     1131       -2     
  Branches      412      411       -1     
==========================================
- Hits         1097     1095       -2     
  Misses         27       27              
  Partials        9        9              
Impacted Files Coverage Δ
src/runtime/sourceMaps.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@alexander-akait alexander-akait left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid it can break a logic for developer without sass-loader and less-loader (i.e. just pure CSS), why do not disable generation of sourceURL on sass-loader side?

@alexander-akait

Copy link
Copy Markdown
Member

hm, maybe you are right, I looked at code and found we already built-in source map:

const data = `sourceMappingURL=data:application/json;charset=utf-8;base64,${base64}`;
const sourceMapping = `/*# ${data} */`;

and then duplicate:

const sourceURLs = cssMapping.sources.map(
  (source) => `/*# sourceURL=${cssMapping.sourceRoot || ""}${source} */`
);

Not sure why we are doing it, I want to look at github history and found why it was implemented

@alan-agius4

Copy link
Copy Markdown
Contributor Author

Correct, I did look into the git history before doing the PR to see the reasoning behind this. But I didn’t find anything meaningful. It looks like it was implemented like this from the beginning.

@alexander-akait

Copy link
Copy Markdown
Member

@alan-agius4 That is really strange, no one reports about it before 😃

@alexander-akait

Copy link
Copy Markdown
Member

@alan-agius4 6da7e90, looks like it was implemented for angular 😄

@alexander-akait

Copy link
Copy Markdown
Member

Okay, let's merge and release, I think it should not break the logic, because we really have the same in sourceMappingURL (maybe chrome had bugs with sources before, but I think we can't check)

@alexander-akait
alexander-akait merged commit 962924c into webpack:master Dec 14, 2022
@alan-agius4
alan-agius4 deleted the css-source-url branch December 14, 2022 14:52
@alan-agius4

Copy link
Copy Markdown
Contributor Author

Thanks @alexander-akait

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants