fix(compiler): prevent shimCssText from adding extra blank lines per CSS comment#67735
Conversation
…CSS comment The comment placeholder restoration in `shimCssText` appended an unconditional `+ '\n'` to each non-hash comment replacement. Because `_commentRe` does not consume the newline that follows a comment in the source, that newline already remains in `cssText`. The extra `'\n'` was therefore inserted on top of the existing one, shifting every line after each comment down by one. In files with many comments (e.g. large SCSS preambles) this shifts all subsequent CSS rules far enough that the CSS sourcemap — generated before `shimCssText` runs — points to completely wrong source locations in browser DevTools. The fix is to drop the `+ '\n'`; internal newlines within a multi-line comment are still preserved via `_newLinesRe`, and the trailing newline that follows the comment in `cssText` is already present without any extra injection.
angular-robot
Bot
added
the
area: compiler
|
@alan-agius4 Would you mind taking a look too? This change appears to make sense to me, but I'd appreciate your review too since you looked at this most recently. It appears we were adding newlines unconditionally for comments rather than just comments containing newlines, which was throwing off our line count in the sourcemaps. |
|
@mattlewis92 Thanks for the PR, BTW! |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
alan-agius4
added
the
target: patch
There was a problem hiding this comment.
I tested a bit more with this change and the line numbers do not match not using view encapsulation emulated without the additional \n.
Prior to this change
With this change
The source line code is on 6
Can you please provide a reproduction of when the sourcemaps line numbers are not matching?
|
@alan-agius4 that looks correct to me? The source map is meant to point to the start of the declaration, which in your example is Clicking on a declaration is meant to take you to the start of the declaration and not the first rule inside of it: |
|
Oh you are right :) |
|
caretaker note: G3 failure is pre-existing. https://fusion2.corp.google.com/presubmit/886663941/OCL:886663941:BASE:886691226:1774003781698:18cd6b3a/targets |
alan-agius4
added
the
merge: caretaker note
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
4 participants
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Component sourcemaps using emulated style encapsulation were wrong due to
encapsulateStyleadding new lines to CSS files which makes the sourcemap line numbers incorrect, thus clicking to navigate to sourcemaps in devtools would take you to the wrong locationIssue Number: N/A
What is the new behavior?
Component sourcemaps are correct - we patched this into our angular version and verified this does indeed fix the issue we were experiencing.
Does this PR introduce a breaking change?
Other information