fix(compiler): do not remove comments in component styles (#50346)

alan-agius4 · dylhunn · commit 540e643347b9 · 2023-06-01T08:45:11.000-07:00
Prior to this commit, comments in CSS were being removed. This caused inline sourcemaps to break to the shift in lines. This caused sourcemaps to break in the ESBuild based builder as this always adds comments at the top of the file with the filename. Example ```css /* src/app/app.component.scss */ * { color: red; background: transparent; } /*# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIndlYnBhY2s6Ly8uL3NyYy9hcHAvYXBwLmNvbXBvbmVudC5zY3NzIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiJBQUNBOzs7Ozs7Ozs7Q0FBQTtBQVdBO0VBQ0UsVUFBQTtFQUNBLHVCQUFBO0FBREYiLCJzb3VyY2VzQ29udGVudCI6WyIvL01FRElBIFFVRVJZIE1BTkFHRVJcbi8qXG4gIDAgLSA2MDA6IFBob25lXG4gIDYwMCAtIDkwMDogVGFibGV0IHBvcnRyYWl0XG4gIDkwMCAtIDEyMDA6IFRhYmxldCBsYW5kc2NhcGVcbiAgMTIwMCAtIDE4MDA6IE5vcm1hbCBzdHlsZXNcbiAgMTgwMCsgOiBCaWcgRGVza3RvcFxuICAxZW0gPSAxNnB4XG4gIFRoZSBzbWFsbGVyIGRldmljZSBydWxlcyBhbHdheXMgc2hvdWxkIHdyaXRlIGJlbG93IHRoZSBiaWdnZXIgZGV2aWNlIHJ1bGVzXG4gIEZpeGluZyBPcmRlciA9PiBCYXNlICsgVHlwb2dyYXBoeSA+PiBHZW5lcmFsIExheW91dCArIEdyaWQgPj4gUGFnZSBMYXlvdXQgKyBDb21wb25lbnRcbiovXG5cbioge1xuICBjb2xvcjogcmVkO1xuICBiYWNrZ3JvdW5kOiB0cmFuc3BhcmVudDtcbn1cbiJdLCJzb3VyY2VSb290IjoiIn0= */ ``` Closes #50308 PR Close #50346
diff --git a/packages/compiler/src/shadow_css.ts b/packages/compiler/src/shadow_css.ts
@@ -138,12 +138,30 @@ export class ShadowCss {
    * The hostSelector is the attribute added to the host itself.
    */
   shimCssText(cssText: string, selector: string, hostSelector: string = ''): string {
-    const commentsWithHash = extractCommentsWithHash(cssText);
-    cssText = stripComments(cssText);
-    cssText = this._insertDirectives(cssText);
+    // **NOTE**: Do not strip comments as this will cause component sourcemaps to break
+    // due to shift in lines.
+
+    // Collect comments and replace them with a placeholder, this is done to avoid complicating
+    // the rule parsing RegExp and keep it safer.
+    const comments: string[] = [];
+    cssText = cssText.replace(_commentRe, (m) => {
+      if (m.match(_commentWithHashRe)) {
+        comments.push(m);
+      } else {
+        // Replace non hash comments with empty lines.
+        // This is done so that we do not leak any senstive data in comments.
+        const newLinesMatches = m.match(_newLinesRe);
+        comments.push((newLinesMatches?.join('') ?? '') + '\n');
+      }
 
+      return COMMENT_PLACEHOLDER;
+    });
+
+    cssText = this._insertDirectives(cssText);
     const scopedCssText = this._scopeCssText(cssText, selector, hostSelector);
-    return [scopedCssText, ...commentsWithHash].join('\n');
+    // Add back comments at the original position.
+    let commentIdx = 0;
+    return scopedCssText.replace(_commentWithHashPlaceHolderRe, () => comments[commentIdx++]);
   }
 
   private _insertDirectives(cssText: string): string {
@@ -434,7 +452,7 @@ export class ShadowCss {
     return cssText.replace(_cssColonHostRe, (_, hostSelectors: string, otherSelectors: string) => {
       if (hostSelectors) {
         const convertedSelectors: string[] = [];
-        const hostSelectorArray = hostSelectors.split(',').map(p => p.trim());
+        const hostSelectorArray = hostSelectors.split(',').map((p) => p.trim());
         for (const hostSelector of hostSelectorArray) {
           if (!hostSelector) break;
           const convertedSelector =
@@ -464,7 +482,7 @@ export class ShadowCss {
    * .foo<scopeName> .bar { ... }
    */
   private _convertColonHostContext(cssText: string): string {
-    return cssText.replace(_cssColonHostContextReGlobal, selectorText => {
+    return cssText.replace(_cssColonHostContextReGlobal, (selectorText) => {
       // We have captured a selector that contains a `:host-context` rule.
 
       // For backward compatibility `:host-context` may contain a comma separated list of selectors.
@@ -478,12 +496,12 @@ export class ShadowCss {
       // Execute `_cssColonHostContextRe` over and over until we have extracted all the
       // `:host-context` selectors from this selector.
       let match: RegExpExecArray|null;
-      while (match = _cssColonHostContextRe.exec(selectorText)) {
+      while ((match = _cssColonHostContextRe.exec(selectorText))) {
         // `match` = [':host-context(<selectors>)<rest>', <selectors>, <rest>]
 
         // The `<selectors>` could actually be a comma separated list: `:host-context(.one, .two)`.
         const newContextSelectors =
-            (match[1] ?? '').trim().split(',').map(m => m.trim()).filter(m => m !== '');
+            (match[1] ?? '').trim().split(',').map((m) => m.trim()).filter((m) => m !== '');
 
         // We must duplicate the current selector group for each of these new selectors.
         // For example if the current groups are:
@@ -507,8 +525,7 @@ export class ShadowCss {
         repeatGroups(contextSelectorGroups, newContextSelectors.length);
         for (let i = 0; i < newContextSelectors.length; i++) {
           for (let j = 0; j < contextSelectorGroupsLength; j++) {
-            contextSelectorGroups[j + (i * contextSelectorGroupsLength)].push(
-                newContextSelectors[i]);
+            contextSelectorGroups[j + i * contextSelectorGroupsLength].push(newContextSelectors[i]);
           }
         }
 
@@ -520,7 +537,7 @@ export class ShadowCss {
       // selectors that `:host-context` can match. See `combineHostContextSelectors()` for more
       // info about how this is done.
       return contextSelectorGroups
-          .map(contextSelectors => combineHostContextSelectors(contextSelectors, selectorText))
+          .map((contextSelectors) => combineHostContextSelectors(contextSelectors, selectorText))
           .join(', ');
     });
   }
@@ -574,7 +591,7 @@ export class ShadowCss {
    * ```
    */
   private _stripScopingSelectors(cssText: string): string {
-    return processRules(cssText, rule => {
+    return processRules(cssText, (rule) => {
       const selector = rule.selector.replace(_shadowDeepSelectors, ' ')
                            .replace(_polyfillHostNoCombinatorRe, ' ');
       return new CssRule(selector, rule.content);
@@ -583,7 +600,7 @@ export class ShadowCss {
 
   private _scopeSelector(selector: string, scopeSelector: string, hostSelector: string): string {
     return selector.split(',')
-        .map(part => part.trim().split(_shadowDeepSelectors))
+        .map((part) => part.trim().split(_shadowDeepSelectors))
         .map((deepParts) => {
           const [shallowPart, ...otherParts] = deepParts;
           const applyScope = (shallowPart: string) => {
@@ -802,22 +819,18 @@ const _polyfillHostRe = /-shadowcsshost/gim;
 const _colonHostRe = /:host/gim;
 const _colonHostContextRe = /:host-context/gim;
 
+const _newLinesRe = /\r?\n/g;
 const _commentRe = /\/\*[\s\S]*?\*\//g;
+const _commentWithHashRe = /\/\*\s*#\s*source(Mapping)?URL=/g;
+const COMMENT_PLACEHOLDER = '%COMMENT%';
+const _commentWithHashPlaceHolderRe = new RegExp(COMMENT_PLACEHOLDER, 'g');
 
 const _placeholderRe = /__ph-(\d+)__/g;
 
-function stripComments(input: string): string {
-  return input.replace(_commentRe, '');
-}
-
-const _commentWithHashRe = /\/\*\s*#\s*source(Mapping)?URL=[\s\S]+?\*\//g;
-
-function extractCommentsWithHash(input: string): string[] {
-  return input.match(_commentWithHashRe) || [];
-}
-
 const BLOCK_PLACEHOLDER = '%BLOCK%';
-const _ruleRe = /(\s*)([^;\{\}]+?)(\s*)((?:{%BLOCK%}?\s*;?)|(?:\s*;))/g;
+const _ruleRe = new RegExp(
+    `(\\s*(?:${COMMENT_PLACEHOLDER}\\s*)*)([^;\\{\\}]+?)(\\s*)((?:{%BLOCK%}?\\s*;?)|(?:\\s*;))`,
+    'g');
 const CONTENT_PAIRS = new Map([['{', '}']]);
 
 const COMMA_IN_PLACEHOLDER = '%COMMA_IN_PLACEHOLDER%';
@@ -865,6 +878,7 @@ function escapeBlocks(
   let blockStartIndex = -1;
   let openChar: string|undefined;
   let closeChar: string|undefined;
+
   for (let i = 0; i < input.length; i++) {
     const char = input[i];
     if (char === '\\') {
@@ -888,12 +902,14 @@ function escapeBlocks(
       resultParts.push(input.substring(nonBlockStartIndex, blockStartIndex));
     }
   }
+
   if (blockStartIndex !== -1) {
     escapedBlocks.push(input.substring(blockStartIndex));
     resultParts.push(placeholder);
   } else {
     resultParts.push(input.substring(nonBlockStartIndex));
   }
+
   return new StringWithEscapedBlocks(resultParts.join(''), escapedBlocks);
 }
 
@@ -1025,7 +1041,6 @@ function unescapeQuotes(str: string, isQuoted: boolean): string {
  *
  * And so on...
  *
- * @param hostMarker the string that selects the host element.
  * @param contextSelectors an array of context selectors that will be combined.
  * @param otherSelectors the rest of the selectors that are not context selectors.
  */
diff --git a/packages/compiler/test/shadow_css/shadow_css_spec.ts b/packages/compiler/test/shadow_css/shadow_css_spec.ts
@@ -96,30 +96,6 @@ describe('ShadowCss', () => {
     expect(css).toEqualCss('div[contenta] {height:calc(100% - 55px);}');
   });
 
-  it('should strip comments', () => {
-    expect(shim('/* x */b {c}', 'contenta')).toEqualCss('b[contenta] {c}');
-  });
-
-  it('should ignore special characters in comments', () => {
-    expect(shim('/* {;, */b {c}', 'contenta')).toEqualCss('b[contenta] {c}');
-  });
-
-  it('should support multiline comments', () => {
-    expect(shim('/* \n */b {c}', 'contenta')).toEqualCss('b[contenta] {c}');
-  });
-
-  it('should keep sourceMappingURL comments', () => {
-    expect(shim('b {c}/*# sourceMappingURL=data:x */', 'contenta'))
-        .toEqualCss('b[contenta] {c} /*# sourceMappingURL=data:x */');
-    expect(shim('b {c}/* #sourceMappingURL=data:x */', 'contenta'))
-        .toEqualCss('b[contenta] {c} /* #sourceMappingURL=data:x */');
-  });
-
-  it('should keep sourceURL comments', () => {
-    expect(shim('/*# sourceMappingURL=data:x */b {c}/*# sourceURL=xxx */', 'contenta'))
-        .toEqualCss('b[contenta] {c} /*# sourceMappingURL=data:x */ /*# sourceURL=xxx */');
-  });
-
   it('should shim rules with quoted content', () => {
     const styleStr = 'div {background-image: url("a.jpg"); color: red;}';
     const css = shim(styleStr, 'contenta');
@@ -137,4 +113,33 @@ describe('ShadowCss', () => {
     const css = shim(styleStr, 'contenta');
     expect(css).toEqualCss('div[contenta]::after { content:"{}"}');
   });
+
+  describe('comments', () => {
+    // Comments should be kept in the same position as otherwise inline sourcemaps break due to
+    // shift in lines.
+    it('should replace multiline comments with newline', () => {
+      expect(shim('/* b {c} */ b {c}', 'contenta')).toBe('\n b[contenta] {c}');
+    });
+
+    it('should replace multiline comments with newline in the original position', () => {
+      expect(shim('/* b {c}\n */ b {c}', 'contenta')).toBe('\n\n b[contenta] {c}');
+    });
+
+    it('should replace comments with newline in the original position', () => {
+      expect(shim('/* b {c} */ b {c} /* a {c} */ a {c}', 'contenta'))
+          .toBe('\n b[contenta] {c} \n a[contenta] {c}');
+    });
+
+    it('should keep sourceMappingURL comments', () => {
+      expect(shim('b {c} /*# sourceMappingURL=data:x */', 'contenta'))
+          .toBe('b[contenta] {c} /*# sourceMappingURL=data:x */');
+      expect(shim('b {c}/* #sourceMappingURL=data:x */', 'contenta'))
+          .toBe('b[contenta] {c}/* #sourceMappingURL=data:x */');
+    });
+
+    it('should handle adjacent comments', () => {
+      expect(shim('/* comment 1 */ /* comment 2 */ b {c}', 'contenta'))
+          .toBe('\n \n b[contenta] {c}');
+    });
+  });
 });
