Skip to content

fix(cdk/schematics): errors when attempting to read some files #19783

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

Merged
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
36 changes: 21 additions & 15 deletions src/cdk/schematics/update-tool/component-resource-collector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,11 @@ export class ComponentResourceCollector {
property.initializer.elements.forEach(el => {
if (ts.isStringLiteralLike(el)) {
const stylesheetPath = this._fileSystem.resolve(sourceFileDirPath, el.text);
const stylesheet = this.resolveExternalStylesheet(stylesheetPath, node);

// In case the stylesheet does not exist in the file system, skip it gracefully.
if (!this._fileSystem.exists(stylesheetPath)) {
return;
if (stylesheet) {
Copy link
Member

Choose a reason for hiding this comment

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

The change in general is good (enabling strict), but I wonder how this issue surfaces? Do we understand how this can happen? We asserted that a file exists in the virtual tree, and then read from it? Reading the file from the tree should not be null, or is this an edge case when a file is not readable somehow?

https://cs.opensource.google/angular/angular-cli/+/master:packages/angular_devkit/schematics/src/tree/host-tree.ts;l=282-291?q=read&ss=angular%2Fangular-cli

Copy link
Member Author

Choose a reason for hiding this comment

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

The assertion here was fine, but I think it failed on the one below where we didn't have an assertion.

Copy link
Member

@devversion devversion Jun 29, 2020

Choose a reason for hiding this comment

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

Unless I miss something, we had a check before we invoked resolveExternalStylesheet. And the other one just accepted a glob of source files which are guaranteed to exist in FS.

Maybe the glob returned source files which exist on FS but are not available in the virtual file system. That would seem most likely. It would be good understanding this as it could potentially be a bug in the devkit, or general reasoning required for future schematic work.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it depends on whether the resolve call here corresponds to a path.resolve from Node which won't check the file system, or whether it's something different that actually does the lookup. If it's the latter, your explanation sounds plausible. Either way, I think that we should get this in for the next release so people that are updating to v10 don't run into it.

Copy link
Member

Choose a reason for hiding this comment

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

The resolve call should not be relevant here. The list of additionalStylesheetPaths is based on a real file system query, and resolve just turns the absolute FS path into a virtual FS workspace relative path.

So there are two options: Either the files are somehow not readable and are not added to the virtual FS tree, or our path resolution logic is flawed. And this is the point/issue I think we should figure out. Fixing the issue is great, but understanding the root cause seems beneficial. To be clear: From the beginning on I agreed that this is a good change to land either way, but it would be still worth figuring this out I assume. I'll ask the issue reporter for more information.

Also I think since this has been reported only once and seems very edgy/surprising, this is probably not P2 worthy.

this.resolvedStylesheets.push(stylesheet);
}

this.resolvedStylesheets.push(this.resolveExternalStylesheet(stylesheetPath, node));
}
});
}
Expand All @@ -155,24 +153,32 @@ export class ComponentResourceCollector {
}

const fileContent = this._fileSystem.read(templatePath);
const lineStartsMap = computeLineStartsMap(fileContent);

this.resolvedTemplates.push({
filePath: templatePath,
container: node,
content: fileContent,
inline: false,
start: 0,
getCharacterAndLineOfPosition: pos => getLineAndCharacterFromPosition(lineStartsMap, pos),
});
if (fileContent) {
const lineStartsMap = computeLineStartsMap(fileContent);

this.resolvedTemplates.push({
filePath: templatePath,
container: node,
content: fileContent,
inline: false,
start: 0,
getCharacterAndLineOfPosition: p => getLineAndCharacterFromPosition(lineStartsMap, p),
});
}
}
});
}

/** Resolves an external stylesheet by reading its content and computing line mappings. */
resolveExternalStylesheet(filePath: WorkspacePath, container: ts.ClassDeclaration|null):
ResolvedResource {
ResolvedResource|null {
const fileContent = this._fileSystem.read(filePath);

if (!fileContent) {
return null;
}

const lineStartsMap = computeLineStartsMap(fileContent);

return {
Expand Down
6 changes: 3 additions & 3 deletions src/cdk/schematics/update-tool/file-system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,11 @@ export abstract class FileSystem<T = WorkspacePath> {
/** Applies all changes which have been recorded in update recorders. */
abstract commitEdits(): void;
/** Creates a new file with the given content. */
abstract create(filePath: T, content: string);
abstract create(filePath: T, content: string): void;
/** Overwrites an existing file with the given content. */
abstract overwrite(filePath: T, content: string);
abstract overwrite(filePath: T, content: string): void;
/** Deletes the given file. */
abstract delete(filePath: T);
abstract delete(filePath: T): void;
/**
* Resolves given paths to a resolved path in the file system. For example, the devkit
* tree considers the actual workspace directory as file system root.
Expand Down
20 changes: 11 additions & 9 deletions src/cdk/schematics/update-tool/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,15 +102,17 @@ export class UpdateProject<Context> {
// specified in any Angular component. Therefore we allow for additional stylesheets
// being specified. We visit them in each migration unless they have been already
// discovered before as actual component resource.
additionalStylesheetPaths.forEach(filePath => {
const resolvedPath = this._fileSystem.resolve(filePath);
const stylesheet = resourceCollector.resolveExternalStylesheet(resolvedPath, null);
// Do not visit stylesheets which have been referenced from a component.
if (!this._analyzedFiles.has(resolvedPath)) {
migrations.forEach(r => r.visitStylesheet(stylesheet));
this._analyzedFiles.add(resolvedPath);
}
});
if (additionalStylesheetPaths) {
additionalStylesheetPaths.forEach(filePath => {
const resolvedPath = this._fileSystem.resolve(filePath);
const stylesheet = resourceCollector.resolveExternalStylesheet(resolvedPath, null);
// Do not visit stylesheets which have been referenced from a component.
if (!this._analyzedFiles.has(resolvedPath) && stylesheet) {
migrations.forEach(r => r.visitStylesheet(stylesheet));
this._analyzedFiles.add(resolvedPath);
}
});
}

// Call the "postAnalysis" method for each migration.
migrations.forEach(r => r.postAnalysis());
Expand Down
5 changes: 3 additions & 2 deletions src/cdk/schematics/update-tool/target-version.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export enum TargetVersion {
* based on the "TargetVersion" enum.
*/
export function getAllVersionNames(): string[] {
return Object.keys(TargetVersion)
.filter(enumValue => typeof TargetVersion[enumValue] === 'string');
return Object.keys(TargetVersion).filter(enumValue => {
return typeof (TargetVersion as Record<string, string|undefined>)[enumValue] === 'string';
});
}
1 change: 1 addition & 0 deletions src/cdk/schematics/update-tool/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"compilerOptions": {
"strict": true,
"lib": ["es2015"],
"types": ["node", "glob"]
}
Expand Down
10 changes: 3 additions & 7 deletions src/cdk/schematics/update-tool/version-changes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,11 @@ export type ValueOfChanges<T> = T extends VersionChanges<infer X>? X : null;
*/
export function getChangesForTarget<T>(target: TargetVersion, data: VersionChanges<T>): T[] {
if (!data) {
throw new Error(
`No data could be found for target version: ${TargetVersion[target]}`);
const version = (TargetVersion as Record<string, string>)[target];
throw new Error(`No data could be found for target version: ${version}`);
}

if (!data[target]) {
return [];
}

return data[target]!.reduce((result, prData) => result.concat(prData.changes), [] as T[]);
return (data[target] || []).reduce((result, prData) => result.concat(prData.changes), [] as T[]);
}

/**
Expand Down