From 1cb7099c03a463cdece657f8b5b07194308b0e06 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 12 Feb 2020 21:59:55 +0100 Subject: [PATCH] =?UTF-8?q?fix(ng-update):=20avoid=20error=20if=20project?= =?UTF-8?q?=20has=20folder=20ending=20with=E2=80=A6=20(#18454)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, we always queried for `.css` and `.scss` files in the whole workspace. We relied on `glob` for this, but did not disable folder matching. Hence, it could happen that in some cases the migration rule tried reading content from a directory (resulting in `EISDIR` error). Additionally, we always queried for files in the actual workspace root. This has downsides and is not correct because it could mean that stylesheets from other projects are accidentally read (in case of a monorepo for example). Fixes #18434. --- .../misc/global-stylesheets.spec.ts | 24 +++++++++ .../ng-update/upgrade-rules/index.ts | 54 ++++++++++++------- src/cdk/schematics/update-tool/BUILD.bazel | 1 + src/cdk/schematics/update-tool/index.ts | 19 ++++--- .../utils/project-tsconfig-paths.spec.ts | 31 ++++++----- .../utils/project-tsconfig-paths.ts | 44 ++------------- 6 files changed, 92 insertions(+), 81 deletions(-) diff --git a/src/cdk/schematics/ng-update/test-cases/misc/global-stylesheets.spec.ts b/src/cdk/schematics/ng-update/test-cases/misc/global-stylesheets.spec.ts index 611ff84bdf1c..78eabbc3a293 100644 --- a/src/cdk/schematics/ng-update/test-cases/misc/global-stylesheets.spec.ts +++ b/src/cdk/schematics/ng-update/test-cases/misc/global-stylesheets.spec.ts @@ -14,6 +14,7 @@ describe('global stylesheets migration', () => { // be picked up by the update-tool. writeFile(testStylesheetPath, readFileSync(require.resolve('./global-stylesheets-test.scss'), 'utf8')); + writeFile('/projects/cdk-testing/third_party/materialize.css/bundle.css', ''); await runFixers(); @@ -24,4 +25,27 @@ describe('global stylesheets migration', () => { removeTempDir(); }); + + it('should not check stylesheets outside of project target', async () => { + const {runFixers, writeFile, removeTempDir, appTree} = await createTestCaseSetup( + 'migration-v6', migrationCollection, []); + const subProjectStylesheet = '[cdkPortalHost] {\n color: red;\n}\n'; + + writeFile('/sub_project/node_modules/materialize.css/package.json', ''); + writeFile('/sub_project/assets/test.css', subProjectStylesheet); + + let error: any = null; + try { + await runFixers() + } catch (e) { + error = e; + } + + expect(error).toBeNull(); + // if the external stylesheet that is not of a project target would have been checked + // by accident, the stylesheet would differ from the original file content. + expect(appTree.readContent('/sub_project/assets/test.css')).toBe(subProjectStylesheet); + + removeTempDir(); + }); }); diff --git a/src/cdk/schematics/ng-update/upgrade-rules/index.ts b/src/cdk/schematics/ng-update/upgrade-rules/index.ts index 3d9e17442917..8c196e292b97 100644 --- a/src/cdk/schematics/ng-update/upgrade-rules/index.ts +++ b/src/cdk/schematics/ng-update/upgrade-rules/index.ts @@ -8,10 +8,14 @@ import {Rule, SchematicContext, Tree} from '@angular-devkit/schematics'; import {NodePackageInstallTask} from '@angular-devkit/schematics/tasks'; +import {WorkspaceProject} from '@schematics/angular/utility/workspace-models'; import {MigrationRuleType, runMigrationRules} from '../../update-tool'; import {TargetVersion} from '../../update-tool/target-version'; -import {getProjectTsConfigPaths} from '../../utils/project-tsconfig-paths'; +import { + getTargetTsconfigPath, + getWorkspaceConfigGracefully +} from '../../utils/project-tsconfig-paths'; import {RuleUpgradeData} from '../upgrade-data'; import {AttributeSelectorsRule} from './attribute-selectors-rule'; @@ -44,8 +48,8 @@ export const cdkMigrationRules: MigrationRuleType[] = [ type NullableMigrationRule = MigrationRuleType; -type PostMigrationFn = (context: SchematicContext, targetVersion: TargetVersion, - hasFailure: boolean) => void; +type PostMigrationFn = + (context: SchematicContext, targetVersion: TargetVersion, hasFailure: boolean) => void; /** * Creates a Angular schematic rule that runs the upgrade for the @@ -56,12 +60,10 @@ export function createUpgradeRule( onMigrationCompleteFn?: PostMigrationFn): Rule { return async (tree: Tree, context: SchematicContext) => { const logger = context.logger; - const {buildPaths, testPaths} = getProjectTsConfigPaths(tree); + const workspace = getWorkspaceConfigGracefully(tree); - if (!buildPaths.length && !testPaths.length) { - // We don't want to throw here because it would mean that other migrations in the - // pipeline don't run either. Rather print an error message. - logger.error('Could not find any TypeScript project in the CLI workspace configuration.'); + if (workspace === null) { + logger.error('Could not find workspace configuration file.'); return; } @@ -69,22 +71,36 @@ export function createUpgradeRule( // necessary because multiple TypeScript projects can contain the same source file and // we don't want to check these again, as this would result in duplicated failure messages. const analyzedFiles = new Set(); + const projectNames = Object.keys(workspace.projects); const rules = [...cdkMigrationRules, ...extraRules]; let hasRuleFailures = false; - const runMigration = (tsconfigPath: string, isTestTarget: boolean) => { - const result = runMigrationRules( - tree, context.logger, tsconfigPath, isTestTarget, targetVersion, - rules, upgradeData, analyzedFiles); - - hasRuleFailures = hasRuleFailures || result.hasFailures; - }; - - buildPaths.forEach(p => runMigration(p, false)); - testPaths.forEach(p => runMigration(p, true)); + const runMigration = + (project: WorkspaceProject, tsconfigPath: string, isTestTarget: boolean) => { + const result = runMigrationRules( + project, tree, context.logger, tsconfigPath, isTestTarget, targetVersion, rules, + upgradeData, analyzedFiles); + hasRuleFailures = hasRuleFailures || result.hasFailures; + }; + + for (const projectName of projectNames) { + const project = workspace.projects[projectName]; + const buildTsconfigPath = getTargetTsconfigPath(project, 'build'); + const testTsconfigPath = getTargetTsconfigPath(project, 'test'); + + if (!buildTsconfigPath && !testTsconfigPath) { + logger.warn(`Could not find TypeScript project for project: ${projectName}`); + continue; + } + if (buildTsconfigPath !== null) { + runMigration(project, buildTsconfigPath, false); + } + if (testTsconfigPath !== null) { + runMigration(project, testTsconfigPath, true); + } + } let runPackageManager = false; - // Run the global post migration static members for all migration rules. rules.forEach(rule => { const actionResult = rule.globalPostMigration(tree, context); diff --git a/src/cdk/schematics/update-tool/BUILD.bazel b/src/cdk/schematics/update-tool/BUILD.bazel index 81810be1ebaf..8f4ccd14214f 100644 --- a/src/cdk/schematics/update-tool/BUILD.bazel +++ b/src/cdk/schematics/update-tool/BUILD.bazel @@ -10,6 +10,7 @@ ts_library( deps = [ "@npm//@angular-devkit/core", "@npm//@angular-devkit/schematics", + "@npm//@schematics/angular", "@npm//@types/glob", "@npm//@types/node", "@npm//typescript", diff --git a/src/cdk/schematics/update-tool/index.ts b/src/cdk/schematics/update-tool/index.ts index 5494a1ea14a7..cec95105a7b8 100644 --- a/src/cdk/schematics/update-tool/index.ts +++ b/src/cdk/schematics/update-tool/index.ts @@ -8,8 +8,9 @@ import {logging, normalize} from '@angular-devkit/core'; import {Tree, UpdateRecorder} from '@angular-devkit/schematics'; +import {WorkspaceProject} from '@schematics/angular/utility/workspace-models'; import {sync as globSync} from 'glob'; -import {dirname, relative} from 'path'; +import {dirname, join, relative} from 'path'; import * as ts from 'typescript'; import {ComponentResourceCollector} from './component-resource-collector'; @@ -18,19 +19,20 @@ import {TargetVersion} from './target-version'; import {parseTsconfigFile} from './utils/parse-tsconfig'; export type Constructor = (new (...args: any[]) => T); -export type MigrationRuleType = Constructor> - & {[m in keyof typeof MigrationRule]: (typeof MigrationRule)[m]}; +export type MigrationRuleType = + Constructor>&{[m in keyof typeof MigrationRule]: (typeof MigrationRule)[m]}; export function runMigrationRules( - tree: Tree, logger: logging.LoggerApi, tsconfigPath: string, isTestTarget: boolean, - targetVersion: TargetVersion, ruleTypes: MigrationRuleType[], upgradeData: T, - analyzedFiles: Set): {hasFailures: boolean} { + project: WorkspaceProject, tree: Tree, logger: logging.LoggerApi, tsconfigPath: string, + isTestTarget: boolean, targetVersion: TargetVersion, ruleTypes: MigrationRuleType[], + upgradeData: T, analyzedFiles: Set): {hasFailures: boolean} { // The CLI uses the working directory as the base directory for the // virtual file system tree. const basePath = process.cwd(); const parsed = parseTsconfigFile(tsconfigPath, dirname(tsconfigPath)); const host = ts.createCompilerHost(parsed.options, true); + const projectFsPath = join(basePath, project.root); // We need to overwrite the host "readFile" method, as we want the TypeScript // program to be based on the file contents in the virtual file tree. @@ -94,8 +96,9 @@ export function runMigrationRules( // In some applications, developers will have global stylesheets which are not specified in any // Angular component. Therefore we glob up all CSS and SCSS files outside of node_modules and // dist. The files will be read by the individual stylesheet rules and checked. - // TODO(devversion): double-check if we can solve this in a more elegant way. - globSync('!(node_modules|dist)/**/*.+(css|scss)', {absolute: true, cwd: basePath}) + // TODO: rework this to collect external/global stylesheets from the workspace config. COMP-280. + globSync( + '!(node_modules|dist)/**/*.+(css|scss)', {absolute: true, cwd: projectFsPath, nodir: true}) .filter(filePath => !resourceCollector.resolvedStylesheets.some(s => s.filePath === filePath)) .forEach(filePath => { const stylesheet = resourceCollector.resolveExternalStylesheet(filePath, null); diff --git a/src/cdk/schematics/utils/project-tsconfig-paths.spec.ts b/src/cdk/schematics/utils/project-tsconfig-paths.spec.ts index e67faec130e1..eb9664781a26 100644 --- a/src/cdk/schematics/utils/project-tsconfig-paths.spec.ts +++ b/src/cdk/schematics/utils/project-tsconfig-paths.spec.ts @@ -1,6 +1,6 @@ import {HostTree} from '@angular-devkit/schematics'; import {UnitTestTree} from '@angular-devkit/schematics/testing'; -import {getProjectTsConfigPaths} from './project-tsconfig-paths'; +import {getTargetTsconfigPath, getWorkspaceConfigGracefully} from './project-tsconfig-paths'; describe('project tsconfig paths', () => { let testTree: UnitTestTree; @@ -14,7 +14,10 @@ describe('project tsconfig paths', () => { {my_name: {architect: {build: {options: {tsConfig: './my-custom-config.json'}}}}} })); - expect(getProjectTsConfigPaths(testTree).buildPaths).toEqual(['my-custom-config.json']); + const config = getWorkspaceConfigGracefully(testTree); + expect(config).not.toBeNull(); + expect(getTargetTsconfigPath(config!.projects['my_name'], 'build')) + .toEqual('my-custom-config.json'); }); it('should be able to read workspace configuration which is using JSON5 features', () => { @@ -34,7 +37,10 @@ describe('project tsconfig paths', () => { }, }`); - expect(getProjectTsConfigPaths(testTree).buildPaths).toEqual(['my-build-config.json']); + const config = getWorkspaceConfigGracefully(testTree); + expect(config).not.toBeNull(); + expect(getTargetTsconfigPath(config!.projects['with_tests'], 'build')) + .toEqual('my-build-config.json'); }); it('should detect test tsconfig path inside of angular.json file', () => { @@ -43,7 +49,10 @@ describe('project tsconfig paths', () => { projects: {my_name: {architect: {test: {options: {tsConfig: './my-test-config.json'}}}}} })); - expect(getProjectTsConfigPaths(testTree).testPaths).toEqual(['my-test-config.json']); + const config = getWorkspaceConfigGracefully(testTree); + expect(config).not.toBeNull(); + expect(getTargetTsconfigPath(config!.projects['my_name'], 'test')) + .toEqual('my-test-config.json'); }); it('should detect test tsconfig path inside of .angular.json file', () => { @@ -53,15 +62,9 @@ describe('project tsconfig paths', () => { {with_tests: {architect: {test: {options: {tsConfig: './my-test-config.json'}}}}} })); - expect(getProjectTsConfigPaths(testTree).testPaths).toEqual(['my-test-config.json']); - }); - - it('should not return duplicate tsconfig files', () => { - testTree.create('/tsconfig.json', ''); - testTree.create('/.angular.json', JSON.stringify({ - projects: {app: {architect: {build: {options: {tsConfig: 'tsconfig.json'}}}}} - })); - - expect(getProjectTsConfigPaths(testTree).buildPaths).toEqual(['tsconfig.json']); + const config = getWorkspaceConfigGracefully(testTree); + expect(config).not.toBeNull(); + expect(getTargetTsconfigPath(config!.projects['with_tests'], 'test')) + .toEqual('my-test-config.json'); }); }); diff --git a/src/cdk/schematics/utils/project-tsconfig-paths.ts b/src/cdk/schematics/utils/project-tsconfig-paths.ts index b4335ecf337a..da867b37da04 100644 --- a/src/cdk/schematics/utils/project-tsconfig-paths.ts +++ b/src/cdk/schematics/utils/project-tsconfig-paths.ts @@ -8,49 +8,13 @@ import {JsonParseMode, normalize, parseJson} from '@angular-devkit/core'; import {Tree} from '@angular-devkit/schematics'; -import {WorkspaceProject} from '@schematics/angular/utility/workspace-models'; +import {WorkspaceProject, WorkspaceSchema} from '@schematics/angular/utility/workspace-models'; /** Name of the default Angular CLI workspace configuration files. */ const defaultWorkspaceConfigPaths = ['/angular.json', '/.angular.json']; -/** - * Gets all tsconfig paths from a CLI project by reading the workspace configuration - * and looking for common tsconfig locations. - */ -export function getProjectTsConfigPaths(tree: Tree): {buildPaths: string[], testPaths: string[]} { - // Start with some tsconfig paths that are generally used within CLI projects. Note - // that we are not interested in IDE-specific tsconfig files (e.g. /tsconfig.json) - const buildPaths = new Set([]); - const testPaths = new Set([]); - - // Add any tsconfig directly referenced in a build or test task of the angular.json workspace. - const workspace = getWorkspaceConfigGracefully(tree); - - if (workspace) { - const projects = Object.keys(workspace.projects).map(name => workspace.projects[name]); - for (const project of projects) { - const buildPath = getTargetTsconfigPath(project, 'build'); - const testPath = getTargetTsconfigPath(project, 'test'); - - if (buildPath) { - buildPaths.add(buildPath); - } - - if (testPath) { - testPaths.add(testPath); - } - } - } - - // Filter out tsconfig files that don't exist in the CLI project. - return { - buildPaths: Array.from(buildPaths).filter(p => tree.exists(p)), - testPaths: Array.from(testPaths).filter(p => tree.exists(p)), - }; -} - /** Gets the tsconfig path from the given target within the specified project. */ -function getTargetTsconfigPath(project: WorkspaceProject, targetName: string): string|null { +export function getTargetTsconfigPath(project: WorkspaceProject, targetName: string): string|null { if (project.targets && project.targets[targetName] && project.targets[targetName].options && project.targets[targetName].options.tsConfig) { return normalize(project.targets[targetName].options.tsConfig); @@ -69,7 +33,7 @@ function getTargetTsconfigPath(project: WorkspaceProject, targetName: string): s * versions of the CLI. Also it's important to resolve the workspace gracefully because * the CLI project could be still using `.angular-cli.json` instead of thew new config. */ -function getWorkspaceConfigGracefully(tree: Tree): any { +export function getWorkspaceConfigGracefully(tree: Tree): null|WorkspaceSchema { const path = defaultWorkspaceConfigPaths.find(filePath => tree.exists(filePath)); const configBuffer = tree.read(path!); @@ -80,7 +44,7 @@ function getWorkspaceConfigGracefully(tree: Tree): any { try { // Parse the workspace file as JSON5 which is also supported for CLI // workspace configurations. - return parseJson(configBuffer.toString(), JsonParseMode.Json5); + return parseJson(configBuffer.toString(), JsonParseMode.Json5) as unknown as WorkspaceSchema; } catch (e) { return null; }