8000 fix(migrations): more robust trailing comma removal in unused imports migration by crisbeto · Pull Request #62118 · angular/angular · GitHub
[go: up one dir, main page]

Skip to content

fix(migrations): more robust trailing comma removal in unused imports migration #62118

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

Closed
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import {ErrorCode, FileSystem, ngErrorCode} from '@angular/compiler-cli';
import {DiagnosticCategoryLabel} from '@angular/compiler-cli/src/ngtsc/core/api';
import {ImportManager} from '@angular/compiler-cli/private/migrations';
import {applyImportManagerChanges} from '../../utils/tsurge/helpers/apply_import_manager';
import {getLeadingLineWhitespaceOfNode} from '../../utils/tsurge/helpers/ast/leading_space';

/** Data produced by the migration for each compilation unit. */
export interface CompilationUnitData {
Expand Down Expand Up @@ -306,10 +305,14 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<
replacements.push(
new Replacement(
projectFile(sourceFile, info),
getArrayElementRemovalUpdate(node, parent, sourceText),
getArrayElementRemovalUpdate(node, sourceText),
),
);
});

stripTrailingSameLineCommas(parent, toRemove, sourceText)?.forEach((update) => {
replacements.push(new Replacement(projectFile(sourceFile, info), update));
});
});

// Attempt to clean up unused import declarations. Note that this isn't foolproof, because we
Expand All @@ -333,11 +336,7 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<
}

/** Generates a `TextUpdate` for the removal of an array element. */
function getArrayElementRemovalUpdate(
node: ts.Expression,
parent: ts.ArrayLiteralExpression,
sourceText: string,
): TextUpdate {
function getArrayElementRemovalUpdate(node: ts.Expression, sourceText: string): TextUpdate {
let position = node.getStart();
let end = node.getEnd();
let toInsert = '';
Expand All @@ -358,25 +357,49 @@ function getArrayElementRemovalUpdate(
}
}

// If we're removing the last element in the array, adjust the starting offset so that
// it includes the previous comma on the same line. This avoids turning something like
// `[One, Two, Three]` into `[One,]`. We only do this within the same like, because
// trailing comma at the end of the line is fine.
if (parent.elements[parent.elements.length - 1] === node) {
for (let i = position - 1; i >= 0; i--) {
const char = sourceText[i];
return new TextUpdate({position, end, toInsert});
}

/** Returns `TextUpdate`s that will remove any leftover trailing commas on the same line. */
function stripTrailingSameLineCommas(
node: ts.ArrayLiteralExpression,
toRemove: Set<ts.Expression>,
sourceText: string,
) {
let updates: TextUpdate[] | null = null;

for (let i = 0; i < node.elements.length; i++) {
// Skip over elements that are being removed already.
if (toRemove.has(node.elements[i])) {
continue;
}

// An element might have a trailing comma if all elements after it have been removed.
const mightHaveTrailingComma = node.elements.slice(i + 1).every((e) => toRemove.has(e));

if (!mightHaveTrailingComma) {
continue;
}

const position = node.elements[i].getEnd();
let end = position;

// If the item might have a trailing comma, start looking after it until we hit a line break.
for (let charIndex = position; charIndex < node.getEnd(); charIndex++) {
const char = sourceText[charIndex];

if (char === ',' || char === ' ') {
position--;
end++;
} else {
if (whitespaceOrLineFeed.test(char)) {
// Replace the node with its leading whitespace to preserve the formatting.
// This only needs to happen if we're breaking on a newline.
toInsert = getLeadingLineWhitespaceOfNode(node);
if (char !== '\n' && position !== end) {
updates ??= [];
updates.push(new TextUpdate({position, end, toInsert: ''}));
}

break;
}
}
}

return new TextUpdate({position, end, toInsert});
return updates;
}
Original file line number Diff line number Diff line change
Expand Up @@ -306,4 +306,27 @@ describe('cleanup unused imports schematic', () => {
expect(contents).toContain(' imports: [/* Start */ One/* End */],');
expect(contents).toContain(`import {One} from './directives';`);
});

it('should handle all items except the first one being removed', async () => {
writeFile(
'comp.ts',
`
import {Component} from '@angular/core';
import {One, Two, Three} from './directives';

@Component({
imports: [Three, One, Two],
template: '<div three></div>',
})
export class Comp {}
`,
);

await runMigration();

const contents = tree.readContent('comp.ts');
expect(logs.pop()).toBe('Removed 2 imports in 1 file');
expect(contents).toContain('imports: [Three],');
expect(contents).toContain(`import {Three} from './directives';`);
});
});
Loading
0