From 6f433887e020cd027c148798530a22ba12f7a48b Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 27 Apr 2019 14:00:21 +0200 Subject: [PATCH] fix(ivy): injectable pipe schematic generating incorrect import statements (#30170) Currently the injectable pipe schematic generates invalid imports like `import import { Pipe, PipeTransform, Injectable } from '@angular/core'; from '@angular/core';`. The issue wasn't caught by the unit tests, because the invalid import still contains the valid one. Fixes #30159. PR Close #30170 --- .../google3/injectablePipeRule.ts | 5 ++-- .../migrations/injectable-pipe/index.ts | 5 ++-- .../migrations/injectable-pipe/util.ts | 25 ++++++------------- .../test/google3/injectable_pipe_spec.ts | 5 +++- .../test/injectable_pipe_migration_spec.ts | 6 +++-- 5 files changed, 19 insertions(+), 27 deletions(-) diff --git a/packages/core/schematics/migrations/injectable-pipe/google3/injectablePipeRule.ts b/packages/core/schematics/migrations/injectable-pipe/google3/injectablePipeRule.ts index 62b6523e35..ac3ba8e622 100644 --- a/packages/core/schematics/migrations/injectable-pipe/google3/injectablePipeRule.ts +++ b/packages/core/schematics/migrations/injectable-pipe/google3/injectablePipeRule.ts @@ -10,7 +10,7 @@ import {Replacement, RuleFailure, Rules} from 'tslint'; import * as ts from 'typescript'; import {InjectablePipeVisitor} from '../angular/injectable_pipe_visitor'; -import {INJECTABLE_DECORATOR_NAME, addNamedImport, getNamedImports} from '../util'; +import {INJECTABLE_DECORATOR_NAME, addImport, getNamedImports} from '../util'; /** * TSLint rule that flags `@Pipe` classes that haven't been marked as `@Injectable`. @@ -37,8 +37,7 @@ export class Rule extends Rules.TypedRule { fixes.push(new Replacement( namedImports.getStart(), namedImports.getWidth(), printer.printNode( - ts.EmitHint.Unspecified, - addNamedImport(importDeclarationMissingImport, INJECTABLE_DECORATOR_NAME), + ts.EmitHint.Unspecified, addImport(namedImports, INJECTABLE_DECORATOR_NAME), sourceFile))); } } diff --git a/packages/core/schematics/migrations/injectable-pipe/index.ts b/packages/core/schematics/migrations/injectable-pipe/index.ts index 5015ee71a5..f1b2b96040 100644 --- a/packages/core/schematics/migrations/injectable-pipe/index.ts +++ b/packages/core/schematics/migrations/injectable-pipe/index.ts @@ -14,7 +14,7 @@ import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths'; import {parseTsconfigFile} from '../../utils/typescript/parse_tsconfig'; import {InjectablePipeVisitor} from './angular/injectable_pipe_visitor'; -import {INJECTABLE_DECORATOR_NAME, addNamedImport, getNamedImports} from './util'; +import {INJECTABLE_DECORATOR_NAME, addImport, getNamedImports} from './util'; /** * Runs a migration over a TypeScript project that adds an `@Injectable` @@ -78,8 +78,7 @@ function runInjectablePipeMigration(tree: Tree, tsconfigPath: string, basePath: update.insertRight( namedImports.getStart(), printer.printNode( - ts.EmitHint.Unspecified, - addNamedImport(importDeclarationMissingImport, INJECTABLE_DECORATOR_NAME), + ts.EmitHint.Unspecified, addImport(namedImports, INJECTABLE_DECORATOR_NAME), sourceFile)); } } diff --git a/packages/core/schematics/migrations/injectable-pipe/util.ts b/packages/core/schematics/migrations/injectable-pipe/util.ts index 69e8c32ed4..fd2de7b943 100644 --- a/packages/core/schematics/migrations/injectable-pipe/util.ts +++ b/packages/core/schematics/migrations/injectable-pipe/util.ts @@ -12,28 +12,17 @@ import * as ts from 'typescript'; export const INJECTABLE_DECORATOR_NAME = 'Injectable'; /** - * Adds a named import to an import declaration node. + * Adds an import to a named import node, if the import does not exist already. * @param node Node to which to add the import. * @param importName Name of the import that should be added. */ -export function addNamedImport(node: ts.ImportDeclaration, importName: string) { - const namedImports = getNamedImports(node); +export function addImport(node: ts.NamedImports, importName: string) { + const elements = node.elements; + const isAlreadyImported = elements.some(element => element.name.text === importName); - if (namedImports && ts.isNamedImports(namedImports)) { - const elements = namedImports.elements; - const isAlreadyImported = elements.some(element => element.name.text === importName); - - if (!isAlreadyImported) { - // If there are named imports, there will be an import clause as well. - const importClause = node.importClause !; - const newImportClause = ts.createNamedImports( - [...elements, ts.createImportSpecifier(undefined, ts.createIdentifier(importName))]); - - return ts.updateImportDeclaration( - node, node.decorators, node.modifiers, - ts.updateImportClause(importClause, importClause.name, newImportClause), - node.moduleSpecifier); - } + if (!isAlreadyImported) { + return ts.updateNamedImports( + node, [...elements, ts.createImportSpecifier(undefined, ts.createIdentifier(importName))]); } return node; diff --git a/packages/core/schematics/test/google3/injectable_pipe_spec.ts b/packages/core/schematics/test/google3/injectable_pipe_spec.ts index 6c368f0cd3..6e9c318d89 100644 --- a/packages/core/schematics/test/google3/injectable_pipe_spec.ts +++ b/packages/core/schematics/test/google3/injectable_pipe_spec.ts @@ -85,7 +85,10 @@ describe('Google3 injectable pipe TSLint rule', () => { `); runTSLint(); - expect(getFile('/index.ts')).toContain('import { Pipe, Injectable } from \'@angular/core\''); + + const content = getFile('/index.ts'); + expect(content).toContain('import { Pipe, Injectable } from \'@angular/core\''); + expect((content.match(/import/g) || []).length).toBe(1, 'Expected only one import statement'); }); }); diff --git a/packages/core/schematics/test/injectable_pipe_migration_spec.ts b/packages/core/schematics/test/injectable_pipe_migration_spec.ts index 3a3abba17f..d70b9f5278 100644 --- a/packages/core/schematics/test/injectable_pipe_migration_spec.ts +++ b/packages/core/schematics/test/injectable_pipe_migration_spec.ts @@ -70,8 +70,10 @@ describe('injectable pipe migration', () => { `); runMigration(); - expect(tree.readContent('/index.ts')) - .toContain('import { Pipe, Injectable } from \'@angular/core\''); + + const content = tree.readContent('/index.ts'); + expect(content).toContain('import { Pipe, Injectable } from \'@angular/core\''); + expect((content.match(/import/g) || []).length).toBe(1, 'Expected only one import statement'); }); it('should not add an import for Injectable if it is imported already', () => {