Skip to content

Commit

Permalink
fix(compiler): do not remove comments in component styles (#50346)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@alan-agius4 @dylhunn
alan-agius4 authored and dylhunn committed Jun 1, 2023
1 parent 57d47b3 commit 540e643
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 49 deletions.
65 changes: 40 additions & 25 deletions packages/compiler/src/shadow_css.ts
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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.
Expand All @@ -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:
Expand All @@ -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]);
}
}

Expand All @@ -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(', ');
});
}
Expand Down Expand Up @@ -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);
Expand All @@ -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) => {
Expand Down Expand Up @@ -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%';
Expand Down Expand Up @@ -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 === '\\') {
Expand All @@ -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);
}

Expand Down Expand Up @@ -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.
*/
Expand Down
53 changes: 29 additions & 24 deletions packages/compiler/test/shadow_css/shadow_css_spec.ts
View file
Expand Up @@ -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');
Expand All @@ -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}');
});
});
});

0 comments on commit 540e643

Please sign in to comment.