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.

Sign up for GitHub

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

test: add failing test cases with single file transpilation for JIT transformers #2615

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

test: add failing test cases with single file transpilation for JIT transformers #2615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
174 changes: 174 additions & 0 deletions src/compiler/ng-jest-compiler.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,5 +475,179 @@ describe('NgJestCompiler', () => {
], MyComp);
`);
});

it.failing('should capture constructor type metadata with `emitDecoratorMetadata` enabled', () => {
const { code, diagnostics } = transformCjs(
`
import {Directive} from '@angular/core';
import {MyOtherClass} from './other-file';

@Directive()
export class MyDir {
constructor(other: MyOtherClass) {}
}
`,
{ emitDecoratorMetadata: true },
);

expect(diagnostics.length).toBe(0);
expect(code).toContain('const other_file_1 = require("./other-file");');
// This is actual output code
// `
// MyDir.ctorParameters = () => [
// { type: other_file_1.MyOtherClass }
// ];
// exports.MyDir = MyDir = tslib_1.__decorate([
// (0, core_1.Directive)(),
// tslib_1.__metadata("design:paramtypes", [typeof (_a = typeof other_file_1.MyOtherClass !== \\"undefined\\" && other_file_1.MyOtherClass) === \\"function\\" ? _a : Object]])
// ], MyDir);
// `
expect(code).toContain(dedent`
MyDir.ctorParameters = () => [
{ type: other_file_1.MyOtherClass }
];
exports.MyDir = MyDir = tslib_1.__decorate([
(0, core_1.Directive)(),
tslib_1.__metadata("design:paramtypes", Object])
], MyDir);
`);
});

it.failing('should properly serialize constructor parameter with external qualified name type', () => {
// This test doesn't pass because `transpileModule` can't resolve `import *`
const { code, diagnostics } = transformCjs(`
import {Directive} from '@angular/core';
import * as externalFile from './other-file';

@Directive()
export class MyDir {
constructor(other: externalFile.MyOtherClass) {}
}
`);

expect(diagnostics.length).toBe(0);
expect(code).toContain('const externalFile = require("./other-file");');
// This is actual output code
// `
// MyDir.ctorParameters = () => [
// { type: undefined }
// ];
// exports.MyDir = MyDir = tslib_1.__decorate([
// (0, core_1.Directive)()
// ], MyDir);
// `
expect(code).toContain(dedent`
MyDir.ctorParameters = () => [
{ type: externalFile.MyOtherClass }
];
exports.MyDir = MyDir = tslib_1.__decorate([
(0, core_1.Directive)()
], MyDir);
`);
});

it.failing('should not capture constructor parameter types when not resolving to a value', () => {
// This test doesn't pass because `transpileModule` can't resolve `import *`
const { code, diagnostics } = transformCjs(`
import {Directive, Inject} from '@angular/core';
import * as angular from './external';
import {IOverlay, KeyCodes} from './external';
import TypeFromDefaultImport from './external';

@Directive()
export class MyDir {
constructor(@Inject('$state') param: angular.IState,
@Inject('$overlay') other: IOverlay,
@Inject('$default') fromDefaultImport: TypeFromDefaultImport,
@Inject('$keyCodes') keyCodes: KeyCodes) {}
}
`);

expect(diagnostics.length).toBe(0);
// This is actual output code
// `
// "\\"use strict\\";
// Object.defineProperty(exports, \\"__esModule\\", { value: true });
// exports.MyDir = void 0;
// const tslib_1 = require(\\"tslib\\");
// const core_1 = require(\\"@angular/core\\");
// const external_1 = require(\\"./external\\");
// const external_2 = tslib_1.__importDefault(require(\\"./external\\"));
// let MyDir = class MyDir {
// constructor(param, other, fromDefaultImport, keyCodes) { }
// };
// exports.MyDir = MyDir;
// MyDir.ctorParameters = () => [
// { type: undefined, decorators: [{ type: core_1.Inject, args: ['$state',] }] },
// { type: external_1.IOverlay, decorators: [{ type: core_1.Inject, args: ['$overlay',] }] },
// { type: external_2.default, decorators: [{ type: core_1.Inject, args: ['$default',] }] },
// { type: external_1.KeyCodes, decorators: [{ type: core_1.Inject, args: ['$keyCodes',] }] }
// ];
// exports.MyDir = MyDir = tslib_1.__decorate([
// (0, core_1.Directive)()
// ], MyDir);
// "
// `
expect(code).not.toContain('external');
expect(code).toContain(dedent`
MyDir.ctorParameters = () => [
{ type: undefined, decorators: [{ type: core_1.Inject, args: ['$state',] }] },
{ type: undefined, decorators: [{ type: core_1.Inject, args: ['$overlay',] }] },
{ type: undefined, decorators: [{ type: core_1.Inject, args: ['$default',] }] },
{ type: undefined, decorators: [{ type: core_1.Inject, args: ['$keyCodes',] }] }
Copy link

Choose a reason for hiding this comment

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

If type is expected to be undefined, then why are we trying to resolve the import *? It doesn't seem like we shouldn't need that type? I wonder if the correct behavior on resolution error would just be to fall back to any as the type (and emit undefined)?

Copy link
Collaborator Author

@ahnpnl ahnpnl Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This case produces very interesting output

Object.defineProperty(exports, \"__esModule\", { value: true });
exports.MyDir = void 0;
const tslib_1 = require(\"tslib\");
const core_1 = require(\"@angular/core\");
const external_1 = require(\"./external\");
const external_2 = tslib_1.__importDefault(require(\"./external\"));
let MyDir = class MyDir {
constructor(param, other, fromDefaultImport, keyCodes) { }
};
exports.MyDir = MyDir;
MyDir.ctorParameters = () => [
{ type: undefined, decorators: [{ type: core_1.Inject, args: ['$state',] }] },
{ type: external_1.IOverlay, decorators: [{ type: core_1.Inject, args: ['$overlay',] }] },
{ type: external_2.default, decorators: [{ type: core_1.Inject, args: ['$default',] }] },
{ type: external_1.KeyCodes, decorators: [{ type: core_1.Inject, args: ['$keyCodes',] }] }
];
exports.MyDir = MyDir = tslib_1.__decorate([
(0, core_1.Directive)()
], MyDir);

I saw that the type was resolved correctly when using import name or default import but fail with import namespace.

Copy link

Choose a reason for hiding this comment

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

Hmm, any of these could be an interface which wouldn't be valid in type though. I wonder if the type system is falling back to any because it can't be resolved and then we're assuming the import is a valid value reference. I wonder if the safer assumption would be to fall back to type: undefined in that scenario?

Copy link
Collaborator Author

@ahnpnl ahnpnl Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You were right. When I do typeChecker.getTypeOfSymbol(symbol), it gave me any.

Copy link

Choose a reason for hiding this comment

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

In retrospect, any might not necessarily be a problem. Both type references and value references can be typed any, so that might be fine. I wonder if it's possible to ask TS whether or not each import is a value reference or a type reference. I'm guessing it's assuming imported symbols are a value reference when it can't resolve the import and we'd probably want to reduce that assumption to a type reference in this scenario.

Unfortunately I'm not very familiar with the TS API and docs are pretty lacking, so I'm not sure about the best way to verify that.

Copy link

Choose a reason for hiding this comment

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

We could probably work around this problem a different way. Since this is the Angular decorator transform, we know a few things:

  1. constructor parameters without @Inject must reference a type declaration that's also a value (and thus it's fine to "assume" when transpiling).
  2. constructor parameters with @Inject don't need a type output at all, and we can unconditionally generate type: undefined.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the 2nd case, can we unconditionally NOT generating undefined so that it’s possible to run tests and leave it up to Jest runtime to validate if that is valid runtime value?

For example:

import foo from ‘./foo’ would resolve in type as type: foo.<something>

And the same applies to import * as foo from ‘./foo’

If we set type: undefined, it will break all existing tests and it makes it not possible to run tests in transpilation mode because there will be error like Can't resolve all parameters for AppComponent: (?) which I observe when debugging this issue #963

];
exports.MyDir = MyDir = tslib_1.__decorate([
(0, core_1.Directive)()
], MyDir);
`);
});

it.failing(
'should allow for type-only references to be removed with `emitDecoratorMetadata` from custom decorators',
() => {
const { code, diagnostics } = transformCjs(
`
import { ExternalInterface } from './external-interface';

export function CustomDecorator() {
return <T>(target, propertyKey, descriptor: TypedPropertyDescriptor<T>) => {}
}

export class Foo {
@CustomDecorator() static test(): ExternalInterface { return {}; }
}
`,
{ emitDecoratorMetadata: true },
);

expect(diagnostics.length).toBe(0);
// This is actual output code
// `
// "\\"use strict\\";
// var _a;
// Object.defineProperty(exports, \\"__esModule\\", { value: true });
// exports.Foo = void 0;
// exports.CustomDecorator = CustomDecorator;
// const tslib_1 = require(\\"tslib\\");
// const external_interface_1 = require(\\"./external-interface\\");
// function CustomDecorator() {
// return (target, propertyKey, descriptor) => { };
// }
// class Foo {
// static test() { return {}; }
// }
// exports.Foo = Foo;
// tslib_1.__decorate([
// CustomDecorator(),
// tslib_1.__metadata(\\"design:type\\", Function),
// tslib_1.__metadata(\\"design:paramtypes\\", []),
// tslib_1.__metadata(\\"design:returntype\\", typeof (_a = typeof external_interface_1.ExternalInterface !== \\"undefined\\" && external_interface_1.ExternalInterface) === \\"function\\" ? _a : Object)
// ], Foo, \\"test\\", null);
// "
// `
expect(code).not.toContain('ExternalInterface');
expect(code).toContain('metadata("design:returntype", Object)');
},
);
});
});
6 changes: 3 additions & 3 deletions src/compiler/ng-jest-compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import os from 'node:os';
import path from 'node:path';
import os from 'os';
import path from 'path';

import { type TsJestAstTransformer, TsCompiler, type ConfigSet } from 'ts-jest';
import type ts from 'typescript';
import type * as ts from 'typescript';

import { angularJitApplicationTransform } from '../transformers/jit_transform';
import { replaceResources } from '../transformers/replace-resources';
Expand Down