-
Notifications
You must be signed in to change notification settings - Fork 312
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
feat: angular ctor params transformer #406
Conversation
this introduces the usage of the @ngtools/webpack ctor-params transformer to downlevel angular decorators to static properties
dca54f8
to
e157b78
Compare
src/TransformUtils.ts
Outdated
@@ -5,6 +5,11 @@ import * as TS from 'typescript'; | |||
// inside the ConfigSet right | |||
export interface ConfigSet { | |||
compilerModule: typeof TS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the const ts = configSet.compilerModule
above is redundant and can be removed, this compilerModule
can be removed too. In the latest version of ts-jest
, this property is no longer exposed as public typings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean by this that configSet.compilerModule
equals configSet.tsCompiler
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually those return 2 different things, compilerModule
returns the current typescript package of the project while tsCompiler
returns the compiler instance created internally in ts-jest
.
But in AngularConstructorParametersTransformers.ts
, compilerModule
isn't used anywhere so I think this property compilerModule
can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the other transformers we still need functionality like configSet.compilerModule.getMutableClone(node)
, which was usually injected by the compilerModule
, being an instance of typescript
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so I think ts-jest
should expose that one again in public typings 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this would be great: compilerModule: typeof TypeScript
where import * as TypeScript from 'typescript'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be like this
export declare class ConfigSet {
readonly parentOptions?: TsJestGlobalOptions | undefined;
get versions(): Record<string, string>;
get compilerModule(): TTypeScript;
get tsCompiler(): TsCompiler;
get tsJestDigest(): string;
readonly logger: Logger;
constructor(jestConfig: Config.ProjectConfig, parentOptions?: TsJestGlobalOptions | undefined, parentLogger?: Logger);
}
where TTypeScript
is
import * as _ts from 'typescript';
export declare type TTypeScript = typeof _ts;
It was public before but then marked as internal 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you assume, that only ts-jest
authors transformers, it is correctly marked as internal 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I will extend the type with the compilerModule
inline. Let me know when there's a new version out with the public typings!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, ts-jest
26.1.2 released. It includes the exposed typing for compileModule
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beautiful !
I tried to setup a test, but this requires fiddling a lot with module mocks, specifically to handle the different scenarios of The transformer passes with Angular v9 and v10 projects, so it is sufficient in my opinion. |
Introduces the usage of the @ngtools/webpack ctor-params transformer to downlevel angular decorators to static properties This makes `emitDecoratorMetadata: true` in `tsconfig.spec.json` redundant, but is not compatible with `isolatedModules: true`.
80cdd4d
to
0b5c315
Compare
It's ready to merge. Would you like to have a quick look, @thymikee? |
Ya I think the unit tests can be added later. I ran into similar thing when trying to mock typescript. Ended up I used this way https://github.com/ahnpnl/ts-jest/blob/ec15732c29c6cc2767ef10029f417b50990a6b26/src/compiler/language-service.spec.ts#L114 |
what do you think about in the meantime, we can have 1 example app is ng 9, 1 example app is ng 10 to run the tests for this change ? |
I think that's a good workaround. But instead of introducing e. g.
Or in lerna-fashion:
What do you think? I'd tackle this in a different PR, and this one would be rebased on top of it. |
I like the version like
while we treat test apps as e2e tests which should be in ya it's wise to do in another PR |
Ok, so in this case I would prefer the setup
just to avoid unnecessary folder nesting. The container-folder |
ok let's proceed like that |
/** | ||
* The factory of hoisting transformer factory | ||
* @internal | ||
*/ | ||
export function factory(cs: ConfigSet) { | ||
export function factory(cs: ConfigSet & { compilerModule: TTypeScript }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This factory type isn’t the same as the one in AngularConstructorParametersTransformer.ts. Is it intended ?
/** | ||
* The factory of hoisting transformer factory | ||
* @internal | ||
*/ | ||
export function factory(cs: ConfigSet) { | ||
export function factory(cs: ConfigSet & { compilerModule: TTypeScript }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This factory type isn’t the same as the one in AngularConstructorParametersTransformer.ts. Is it intended ?
we might need to postpone this, see my comment #409 (comment) |
@wtho do you want to include this in 8.3.0 release ? In the meanwhile we are still figuring how to use Angular Compiler API, maybe this can be a quick win ? |
No, I prefer to not merge it, as there is an easy workaround and we would have to remove everything added in this PR again. |
ok 👍 |
is taken care in #562 |
Introduces the usage of the @ngtools/webpack ctor-params transformer to
downlevel angular decorators to static properties
This also is the first usage of an Angular transformer in our setup. By using the compilation tools provided by the Angular Team to compile the code, we ensure the tests treat the code like production code. Furthermore this facilitates keeping up with the Angular Team.
Checklist
README.md
regardingisolatedModules: true
CHANGELOG.md
Closes #288.
Further Discussion
While we are finally able to include Angular transformers, and in this case to circumvent the issue with emitting more metadata than Angular itself, we are still behind Angular Ivy compilation in jest + angular. This could change if we take a better look at the compilation behavior of the AngularCompilerPlugin as well as the used transformers. While partial functionality is definitely not interesting in test scenarios, other features are necessary. There might also be more compiling functionality in
@angular/compiler-cli
. More research has to be done in this field.Note for Compatibility with Angular v10
In Angular v10 the
ctor-parameters.js
disappeared (in this commit). The reference in theAngularConstructorParameters
has to be updated to be compatible with Angular v10 (and therefore incompatible with Angular v9). The current transformer handles both scenarios by trying to loadctor-parameters
first and then falling back to the newercompiler-cli
transformer.