Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ng-update): properly handle update from different working directory #19934

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 22 additions & 18 deletions src/cdk/schematics/ng-update/devkit-file-system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,37 +6,26 @@
* found in the LICENSE file at https://angular.io/license
*/

import {normalize, Path, relative} from '@angular-devkit/core';
import {normalize, Path, PathIsDirectoryException} from '@angular-devkit/core';
import {Tree, UpdateRecorder} from '@angular-devkit/schematics';
import {DirectoryEntry, FileSystem} from '../update-tool/file-system';
import * as path from 'path';
import {FileSystem} from '../update-tool/file-system';

/**
* File system that leverages the virtual tree from the CLI devkit. This file
* system is commonly used by `ng update` migrations that run as part of the
* Angular CLI.
*/
export class DevkitFileSystem extends FileSystem<Path> {
export class DevkitFileSystem extends FileSystem {
private _updateRecorderCache = new Map<string, UpdateRecorder>();
private _workspaceFsPath: Path;

constructor(private _tree: Tree, workspaceFsPath: string) {
constructor(private _tree: Tree) {
super();
this._workspaceFsPath = normalize(workspaceFsPath);
}

resolve(...segments: string[]): Path {
// Note: We use `posix.resolve` as the devkit paths are using posix separators.
const resolvedPath = normalize(path.posix.resolve(...segments.map(normalize)));
// If the resolved path points to the workspace root, then this is an absolute disk
// path and we need to compute a devkit tree relative path.
if (resolvedPath.startsWith(this._workspaceFsPath)) {
return relative(this._workspaceFsPath, resolvedPath);
}
// Otherwise we know that the path is absolute (due to the resolve), and that it
// refers to an absolute devkit tree path (like `/angular.json`). We keep those
// unmodified as they are already resolved workspace paths.
return resolvedPath;
return normalize(path.posix.resolve('/', ...segments.map(normalize)));
}

edit(filePath: Path) {
Expand All @@ -53,8 +42,18 @@ export class DevkitFileSystem extends FileSystem<Path> {
this._updateRecorderCache.clear();
}

exists(filePath: Path) {
return this._tree.exists(filePath);
exists(fileOrDirPath: Path) {
// The devkit tree does not expose an API for checking whether a given
// directory exists. It throws a specific error though if a directory
// is being read as a file. We use that to check if a directory exists.
try {
return this._tree.get(fileOrDirPath) !== null;
} catch (e) {
if (e instanceof PathIsDirectoryException) {
return true;
}
}
return false;
}

overwrite(filePath: Path, content: string) {
Expand All @@ -73,4 +72,9 @@ export class DevkitFileSystem extends FileSystem<Path> {
const buffer = this._tree.read(filePath);
return buffer !== null ? buffer.toString() : null;
}

readDirectory(dirPath: Path): DirectoryEntry {
const {subdirs: directories, subfiles: files} = this._tree.getDir(dirPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something we control, but it's a little weird for this to be called "sub files" since all files are nested in each other. Maybe we can change it if we're changing other things.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's definitely a bit ambiguous. You mean that we create a proposal upstream for them to change it?

return {directories, files};
}
}
39 changes: 16 additions & 23 deletions src/cdk/schematics/ng-update/devkit-migration-rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,16 @@
import {Rule, SchematicContext, Tree} from '@angular-devkit/schematics';
import {NodePackageInstallTask} from '@angular-devkit/schematics/tasks';
import {WorkspaceProject} from '@schematics/angular/utility/workspace-models';
import {sync as globSync} from 'glob';
import {join} from 'path';

import {UpdateProject} from '../update-tool';
import {WorkspacePath} from '../update-tool/file-system';
import {MigrationCtor} from '../update-tool/migration';
import {TargetVersion} from '../update-tool/target-version';
import {WorkspacePath} from '../update-tool/file-system';
import {getTargetTsconfigPath, getWorkspaceConfigGracefully} from '../utils/project-tsconfig-paths';

import {DevkitFileSystem} from './devkit-file-system';
import {DevkitContext, DevkitMigration, DevkitMigrationCtor} from './devkit-migration';
import {findStylesheetFiles} from './find-stylesheets';
import {AttributeSelectorsMigration} from './migrations/attribute-selectors';
import {ClassInheritanceMigration} from './migrations/class-inheritance';
import {ClassNamesMigration} from './migrations/class-names';
Expand Down Expand Up @@ -74,9 +73,7 @@ export function createMigrationSchematicRule(
// 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<WorkspacePath>();
// The CLI uses the working directory as the base directory for the virtual file system tree.
const workspaceFsPath = process.cwd();
const fileSystem = new DevkitFileSystem(tree, workspaceFsPath);
const fileSystem = new DevkitFileSystem(tree);
const projectNames = Object.keys(workspace.projects);
const migrations: NullableDevkitMigration[] = [...cdkMigrations, ...extraMigrations];
let hasFailures = false;
Expand All @@ -90,11 +87,18 @@ export function createMigrationSchematicRule(
logger.warn(`Could not find TypeScript project for project: ${projectName}`);
continue;
}

// 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
// in the project and migrate them if needed.
// TODO: rework this to collect global stylesheets from the workspace config. COMP-280.
const additionalStylesheetPaths = findStylesheetFiles(tree, project.root);

if (buildTsconfigPath !== null) {
runMigrations(project, projectName, buildTsconfigPath, false);
runMigrations(project, projectName, buildTsconfigPath, additionalStylesheetPaths, false);
}
if (testTsconfigPath !== null) {
runMigrations(project, projectName, testTsconfigPath, true);
runMigrations(project, projectName, testTsconfigPath, additionalStylesheetPaths, true);
}
}

Expand Down Expand Up @@ -123,12 +127,10 @@ export function createMigrationSchematicRule(

/** Runs the migrations for the specified workspace project. */
function runMigrations(project: WorkspaceProject, projectName: string,
tsconfigPath: string, isTestTarget: boolean) {
const projectRootFsPath = join(workspaceFsPath, project.root);
const tsconfigFsPath = join(workspaceFsPath, tsconfigPath);
const program = UpdateProject.createProgramFromTsconfig(tsconfigFsPath, fileSystem);
tsconfigPath: WorkspacePath, additionalStylesheetPaths: string[],
isTestTarget: boolean) {
const program = UpdateProject.createProgramFromTsconfig(tsconfigPath, fileSystem);
const updateContext: DevkitContext = {
workspaceFsPath,
isTestTarget,
projectName,
project,
Expand All @@ -143,17 +145,8 @@ export function createMigrationSchematicRule(
context.logger,
);

// 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: rework this to collect global stylesheets from the workspace config. COMP-280.
const additionalStylesheets = globSync(
'!(node_modules|dist)/**/*.+(css|scss)',
{absolute: true, cwd: projectRootFsPath, nodir: true});

const result =
updateProject.migrate(migrations, targetVersion, upgradeData, additionalStylesheets);
updateProject.migrate(migrations, targetVersion, upgradeData, additionalStylesheetPaths);

// Commit all recorded edits in the update recorder. We apply the edits after all
// migrations ran because otherwise offsets in the TypeScript program would be
Expand Down
2 changes: 0 additions & 2 deletions src/cdk/schematics/ng-update/devkit-migration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ export type DevkitContext = {
projectName: string;
/** Workspace project the migrations run against. */
project: WorkspaceProject,
/** Absolute file system path to the workspace */
workspaceFsPath: string,
/** Whether the migrations run for a test target. */
isTestTarget: boolean,
};
Expand Down
30 changes: 30 additions & 0 deletions src/cdk/schematics/ng-update/find-stylesheets.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {join} from '@angular-devkit/core';
import {Tree} from '@angular-devkit/schematics';

/** Regular expression that matches stylesheet paths */
const STYLESHEET_REGEX = /.*\.(css|scss)/;

/** Finds stylesheets in the given directory from within the specified tree. */
export function findStylesheetFiles(tree: Tree, baseDir: string): string[] {
const result: string[] = [];
const visitDir = dirPath => {
const {subfiles, subdirs} = tree.getDir(dirPath);
result.push(...subfiles.filter(f => STYLESHEET_REGEX.test(f)));
subdirs.forEach(fragment => {
// Do not visit directories or files inside node modules or `dist/` folders.
if (fragment !== 'node_modules' && fragment !== 'dist') {
visitDir(join(dirPath, fragment));
}
});
};
visitDir(baseDir);
return result;
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,12 @@ export class AttributeSelectorsMigration extends Migration<UpgradeData> {
}

const literalText = literal.getText();
const filePath = literal.getSourceFile().fileName;
const filePath = this.fileSystem.resolve(literal.getSourceFile().fileName);

this.data.forEach(selector => {
findAllSubstringIndices(literalText, selector.replace)
.map(offset => literal.getStart() + offset)
.forEach(start => this._replaceSelector(
this.fileSystem.resolve(filePath), start, selector));
.forEach(start => this._replaceSelector(filePath, start, selector));
});
}

Expand Down
4 changes: 2 additions & 2 deletions src/cdk/schematics/ng-update/migrations/css-selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export class CssSelectorsMigration extends Migration<UpgradeData> {
}

const textContent = node.getText();
const filePath = node.getSourceFile().fileName;
const filePath = this.fileSystem.resolve(node.getSourceFile().fileName);

this.data.forEach(data => {
if (data.whitelist && !data.whitelist.strings) {
Expand All @@ -70,7 +70,7 @@ export class CssSelectorsMigration extends Migration<UpgradeData> {

findAllSubstringIndices(textContent, data.replace)
.map(offset => node.getStart() + offset)
.forEach(start => this._replaceSelector(this.fileSystem.resolve(filePath), start, data));
.forEach(start => this._replaceSelector(filePath, start, data));
});
}

Expand Down
5 changes: 2 additions & 3 deletions src/cdk/schematics/ng-update/migrations/element-selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,12 @@ export class ElementSelectorsMigration extends Migration<UpgradeData> {
}

const textContent = node.getText();
const filePath = node.getSourceFile().fileName;
const filePath = this.fileSystem.resolve(node.getSourceFile().fileName);

this.data.forEach(selector => {
findAllSubstringIndices(textContent, selector.replace)
.map(offset => node.getStart() + offset)
.forEach(start => this._replaceSelector(
this.fileSystem.resolve(filePath), start, selector));
.forEach(start => this._replaceSelector(filePath, start, selector));
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {createTestCaseSetup} from '../../../testing';
describe('ng-update external resource resolution', () => {

it('should properly resolve referenced resources in components', async () => {
const {runFixers, writeFile, removeTempDir, appTree} = await createTestCaseSetup(
const {runFixers, writeFile, appTree} = await createTestCaseSetup(
'migration-v6', MIGRATION_PATH,
[resolveBazelPath(__dirname, './external-resource-resolution_input.ts')]);

Expand All @@ -24,7 +24,5 @@ describe('ng-update external resource resolution', () => {
.toBe(expected, 'Expected relative paths with parent segments to work.');
expect(appTree.readContent('/projects/cdk-testing/src/test-cases/local.html'))
.toBe(expected, 'Expected relative paths without explicit dot to work.');

removeTempDir();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {createTestCaseSetup} from '../../../testing';
describe('global stylesheets migration', () => {

it('should not check stylesheet twice if referenced in component', async () => {
const {runFixers, writeFile, removeTempDir, appTree} = await createTestCaseSetup(
const {runFixers, writeFile, appTree} = await createTestCaseSetup(
'migration-v6', MIGRATION_PATH,
[resolveBazelPath(__dirname, './global-stylesheets_input.ts')]);

Expand All @@ -25,12 +25,10 @@ describe('global stylesheets migration', () => {
// the same replacements were recorded for the same source file.
expect(appTree.readContent(testStylesheetPath))
.toBe(`[cdkPortalOutlet] {\n color: red;\n}\n`);

removeTempDir();
});

it('should not check stylesheets outside of project target', async () => {
const {runFixers, writeFile, removeTempDir, appTree} = await createTestCaseSetup(
const {runFixers, writeFile, appTree} = await createTestCaseSetup(
'migration-v6', MIGRATION_PATH, []);
const subProjectStylesheet = '[cdkPortalHost] {\n color: red;\n}\n';

Expand All @@ -43,7 +41,5 @@ describe('global stylesheets migration', () => {
// 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();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {createTestCaseSetup} from '../../../testing';

describe('v6 method call checks', () => {
it('should properly report invalid method calls', async () => {
const {runFixers, removeTempDir} = await createTestCaseSetup(
const {runFixers} = await createTestCaseSetup(
'migration-v6', MIGRATION_PATH,
[resolveBazelPath(__dirname, './method-call-checks_input.ts')]);

Expand All @@ -14,7 +14,5 @@ describe('v6 method call checks', () => {
/@15:5 - Found call to "FocusMonitor\.monitor".*renderer.*has been removed/);
expect(logOutput).toMatch(
/@16:5 - Found call to "FocusMonitor\.monitor".*renderer.*has been removed/);

removeTempDir();
});
});
Loading