Skip to content

Commit

Permalink
fix(@ngtools/webpack): perform import eliding before TypeScript trans…
Browse files Browse the repository at this point in the history
…forms

Due to the method used by TypeScript to emit decorator metadata via the `emitDecoratorMetadata` function, the import eliding algorithm within the Angular compiler plugin may errantly remove imports to type metadata included in the emitted decorator calls. By moving the eliding before TypeScript, the eliding can use the Type nodes to analyze for used imports when `emitDecoratorMetadata` is enabled. This fix also reworks the previous fix to prevent the eliding to errantly remove certain factory functions that TypeScript may use in emitted code but are not yet referenced in the input code. The previous fix was to move the eliding after the TypeScript transformations.  The new fix is therefore not as comprehensive as the original but covers the usecase within the originating issue (#13297).

(cherry picked from commit c2a449b)
  • Loading branch information
clydin authored and dgp1130 committed Jan 22, 2021
1 parent efd5724 commit 50106ff
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 3 deletions.
2 changes: 1 addition & 1 deletion packages/ngtools/webpack/src/ivy/transformation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export function createAotTransformers(
const removeNgModuleScope = !options.emitNgModuleScope;
if (removeClassMetadata || removeNgModuleScope) {
// tslint:disable-next-line: no-non-null-assertion
transformers.after!.push(
transformers.before!.push(
removeIvyJitSupportCalls(removeClassMetadata, removeNgModuleScope, getTypeChecker),
);
}
Expand Down
5 changes: 5 additions & 0 deletions packages/ngtools/webpack/src/transformers/elide_imports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ export function elideImports(
}

const isUnused = (node: ts.Identifier) => {
// Do not remove JSX factory imports
if (node.text === compilerOptions.jsxFactory) {
return false;
}

const symbol = typeChecker.getSymbolAtLocation(node);

return symbol && !usedSymbols.has(symbol);
Expand Down
44 changes: 44 additions & 0 deletions packages/ngtools/webpack/src/transformers/elide_imports_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ describe('@ngtools/webpack transformers', () => {
[propName: string]: unknown;
}
`,
'jsx.ts': `
export function createElement() {}
`,
};

it('should remove unused imports', () => {
Expand Down Expand Up @@ -348,6 +351,47 @@ describe('@ngtools/webpack transformers', () => {
});
});

it('keeps jsxFactory imports when configured', () => {
const extraCompilerOptions: ts.CompilerOptions = {
jsxFactory: 'createElement',
experimentalDecorators: true,
jsx: ts.JsxEmit.React,
};

const input = tags.stripIndent`
import { Decorator } from './decorator';
import { Service } from './service';
import { createElement } from './jsx';
const test = <p>123</p>;
@Decorator()
export class Foo {
constructor(param: Service) {
}
}
${dummyNode}
`;

const output = tags.stripIndent`
import { __decorate } from "tslib";
import { Decorator } from './decorator';
import { createElement } from './jsx';
const test = createElement("p", null, "123");
let Foo = class Foo { constructor(param) { } };
Foo = __decorate([ Decorator() ], Foo);
export { Foo };
`;

const { program, compilerHost } = createTypescriptContext(input, additionalFiles, true, extraCompilerOptions, true);
const result = transformTypescript(undefined, [transformer(program)], program, compilerHost);

expect(tags.oneLine`${result}`).toEqual(tags.oneLine`${output}`);
});

describe('should not elide imports decorator type references when emitDecoratorMetadata is true', () => {
const extraCompilerOptions: ts.CompilerOptions = {
emitDecoratorMetadata: true,
Expand Down
6 changes: 4 additions & 2 deletions packages/ngtools/webpack/src/transformers/spec_helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { WebpackCompilerHost } from '../compiler_host';

// Test transform helpers.
const basePath = '/project/src/';
const fileName = basePath + 'test-file.ts';
const basefileName = basePath + 'test-file.ts';
const typeScriptLibFiles = loadTypeScriptLibFiles();
const tsLibFiles = loadTsLibFiles();

Expand All @@ -23,7 +23,9 @@ export function createTypescriptContext(
additionalFiles?: Record<string, string>,
useLibs = false,
extraCompilerOptions: ts.CompilerOptions = {},
jsxFile = false,
) {
const fileName = basefileName + (jsxFile ? 'x' : '');
// Set compiler options.
const compilerOptions: ts.CompilerOptions = {
noEmitOnError: useLibs,
Expand Down Expand Up @@ -107,7 +109,7 @@ export function transformTypescript(
}

// Return the transpiled js.
return compilerHost.readFile(fileName.replace(/\.tsx?$/, '.js'));
return compilerHost.readFile(basefileName.replace(/\.tsx?$/, '.js'));
}

function loadTypeScriptLibFiles(): Record<string, string> {
Expand Down

0 comments on commit 50106ff

Please sign in to comment.