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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove sourceURL from emitted CSS #1487

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

alan-agius4
Copy link
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
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
Copy link
Contributor Author

alan-agius4 commented Dec 13, 2022

@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
Copy link

codecov bot commented Dec 14, 2022

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.

Copy link
Member

@alexander-akait alexander-akait left a comment

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
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
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
Member

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

@alexander-akait
Copy link
Member

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

@alexander-akait
Copy link
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-contrib:master Dec 14, 2022
@alan-agius4 alan-agius4 deleted the css-source-url branch December 14, 2022 14:52
@alan-agius4
Copy link
Contributor Author

Thanks @alexander-akait

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

Successfully merging this pull request may close these issues.

2 participants