Skip to content

Commit 8669606

Browse files
devversionmmalerba
authored andcommitted
refactor(ng-update): do not visit external stylesheets multiple times (#17286)
Besides stylesheets which are referenced in components, we also visit stylesheets which are not referenced by a component. This has been done at some point to capture global stylesheets. Right now we accidentally visit external stylesheets multiple times. This can result in broken stylesheets. Also this commit does some path manipulation cleanup.
1 parent 29be768 commit 8669606

File tree

5 files changed

+47
-6
lines changed

5 files changed

+47
-6
lines changed

src/cdk/schematics/BUILD.bazel

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,5 +89,5 @@ filegroup(
8989
srcs = glob([
9090
"ng-update/test-cases/**/*_input.ts",
9191
"ng-update/test-cases/**/*_expected_output.ts",
92-
]),
92+
]) + ["ng-update/test-cases/misc/global-stylesheets-test.scss"],
9393
)
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
[cdkPortalHost] {
2+
color: red;
3+
}
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import {createTestCaseSetup} from '../../../testing';
2+
import {migrationCollection} from '../index.spec';
3+
import {readFileSync} from 'fs';
4+
5+
describe('global stylesheets migration', () => {
6+
7+
it('should not check stylesheet twice if referenced in component', async () => {
8+
const {runFixers, writeFile, removeTempDir, appTree} = await createTestCaseSetup(
9+
'migration-v6', migrationCollection, [require.resolve('./global-stylesheets_input.ts')]);
10+
11+
const testStylesheetPath = 'projects/cdk-testing/src/test-cases/global-stylesheets-test.scss';
12+
13+
// Copy the test stylesheets file into the test CLI application. That way it will
14+
// be picked up by the update-tool.
15+
writeFile(testStylesheetPath,
16+
readFileSync(require.resolve('./global-stylesheets-test.scss'), 'utf8'));
17+
18+
await runFixers();
19+
20+
// if the external stylesheet would have been checked multiple times, the migrated
21+
// stylesheet would not match the expected output.
22+
expect(appTree.readContent(testStylesheetPath))
23+
.toBe(`[cdkPortalOutlet] {\n color: red;\n}\n`);
24+
25+
removeTempDir();
26+
});
27+
});
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
import {Component} from '@angular/core';
2+
3+
@Component({
4+
template: 'hello',
5+
styleUrls: ['./global-stylesheets-test.scss'],
6+
})
7+
export class MyTestComp {}

src/cdk/schematics/update-tool/index.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,11 @@ export function runMigrationRules<T>(
9595
.filter(filePath => !resourceCollector.resolvedStylesheets.some(s => s.filePath === filePath))
9696
.forEach(filePath => {
9797
const stylesheet = resourceCollector.resolveExternalStylesheet(filePath, null);
98-
rules.forEach(r => r.visitStylesheet(stylesheet));
98+
const relativePath = getProjectRelativePath(filePath);
99+
// do not visit stylesheets which have been referenced from a component.
100+
if (!analyzedFiles.has(relativePath)) {
101+
rules.forEach(r => r.visitStylesheet(stylesheet));
102+
}
99103
});
100104

101105
// Commit all recorded updates in the update recorder. We need to perform the
@@ -110,7 +114,7 @@ export function runMigrationRules<T>(
110114
// In case there are rule failures, print these to the CLI logger as warnings.
111115
if (ruleFailures.length) {
112116
ruleFailures.forEach(({filePath, message, position}) => {
113-
const normalizedFilePath = normalize(relative(basePath, filePath));
117+
const normalizedFilePath = normalize(getProjectRelativePath(filePath));
114118
const lineAndCharacter = `${position.line + 1}:${position.character + 1}`;
115119
logger.warn(`${normalizedFilePath}@${lineAndCharacter} - ${message}`);
116120
});
@@ -119,7 +123,7 @@ export function runMigrationRules<T>(
119123
return !!ruleFailures.length;
120124

121125
function getUpdateRecorder(filePath: string): UpdateRecorder {
122-
const treeFilePath = relative(basePath, filePath);
126+
const treeFilePath = getProjectRelativePath(filePath);
123127
if (updateRecorderCache.has(treeFilePath)) {
124128
return updateRecorderCache.get(treeFilePath)!;
125129
}
@@ -134,8 +138,8 @@ export function runMigrationRules<T>(
134138
resourceCollector.visitNode(node);
135139
}
136140

137-
/** Gets the specified path relative to the project root. */
141+
/** Gets the specified path relative to the project root in POSIX format. */
138142
function getProjectRelativePath(filePath: string) {
139-
return relative(basePath, filePath);
143+
return relative(basePath, filePath).replace(/\\/g, '/');
140144
}
141145
}

0 commit comments

Comments
 (0)