thymikee / jest-preset-angular

Jest configuration preset for Angular projects.
https://thymikee.github.io/jest-preset-angular/
MIT License
885 stars 306 forks source link

[Bug]: type-only imports cause ReferenceError #1199

Closed Simon-Hayden-iteratec closed 1 year ago

Simon-Hayden-iteratec commented 2 years ago

Version

11.0.1

Steps to reproduce

@leosvelperez created a repo showing the failure: https://github.com/leosvelperez/ng-jest-type-issue

Just clone the repo, install depdencies and execute the tests using jest.

(For reference, this bug was originally reported to the Nx team here: https://github.com/nrwl/nx/issues/7845)

Expected behavior

Tests which use import type should pass. See the official Typescript documentation.

Actual behavior

An error is thrown:

  ● AppService › should be created

    ReferenceError: Request is not defined

      1 | import { Inject, Injectable, Optional } from '@angular/core';
    > 2 | import type { Request } from 'express';
        |               ^
      3 | import { REQUEST } from './app.tokens';
      4 |
      5 | @Injectable({

Additional context

The context of this issue is, that we are using Angular Universal for Server Side Rendering. Hence we sometimes have to inject the Browser's request/response into a service or similar. Since we do not want to accidentally bundle express into our Angular app, we want to use type-only imports.

Environment

npx: installed 1 in 0.89s

  System:
    OS: Linux 5.14 Fedora Linux 35 (Workstation Edition)
    CPU: (8) x64 Intel(R) Core(TM) i7-7820HQ CPU @ 2.90GHz
  Binaries:
    Node: 14.17.6 - ~/.nvm/versions/node/v14.17.6/bin/node
    Yarn: 1.22.15 - ~/.nvm/versions/node/v14.17.6/bin/yarn
    npm: 6.14.15 - ~/.nvm/versions/node/v14.17.6/bin/npm
  npmPackages:
    jest: ^27.3.1 => 27.3.1
ahnpnl commented 2 years ago

workaround is you should not use import type if you have anything injected in constructor.

Another workaround is adjust the include field in tsconfig.spec.json to be

{
    //...
   "include": ["src/**/*.ts"]
}
Simon-Hayden-iteratec commented 2 years ago

Thanks for answering so swiftly!

workaround is you should not use import type if you have anything injected in constructor.

Sorry, not quite sure what you mean by that. Should we remove the type from the import type or should we remove the parameter from the constructor? Or just the type of the parameter?

So given the example provided here, what would you change:

import { Inject, Injectable, Optional } from '@angular/core';
import type { Request } from 'express';
import { REQUEST } from './app.tokens';

@Injectable({
  providedIn: 'root',
})
export class AppService {
  constructor(@Inject(REQUEST) @Optional() private request: Request) {}
}

Another workaround is adjust the include field in tsconfig.spec.json to be [...]

I added it to our tsconfig.spec.json and it works! I was just wondering, is there any drawback in doing it that way? Since the default configuration (generated by the nx CLI in our case) omits it, there must be a good reason for it, right? (Sorry if this is a dumb question to ask, jest is not my strong point)

ahnpnl commented 2 years ago

About how the syntax, I meant import { Request } instead of import type { Request }.

The drawback of changing include in tsconfig might be that Jess might start slower. But anyways the current startup speed is already slow. The value of include impacts how many files ts-jest (we use ts-jest to compile) would need to process before the actual tests run.

Simon-Hayden-iteratec commented 2 years ago

Thanks for your explanation.

About how the syntax, I meant import { Request } instead of import type { Request }.

While this would work too - obviously - I'm afraid the Angular compiler would then include the express library in our (frontend) bundle. I might be wrong here, but I think it would easily be possible to "misuse" the import in an unintended way.

Anyway, thanks for confirming the bug :)

ahnpnl commented 2 years ago

I added a bit explanation also in a similar issue.

ghost commented 2 years ago

@ahnpnl Any news about the bug? include didn't help with type-only imports, still can't get import cross nx libraries

JohanHeyvaert commented 2 years ago

I'm experiencing the same problem in our monorepository (Nx 14.3.6, Angular 14.0.3, Jest 28.1.1, Typescript 4.7.4, jest-preset-angular 12.1.0).

This workaround:

ahnpnl commented on Nov 24, 2021 Another workaround is adjust the include field in tsconfig.spec.json to be

{ //... "include": ["src/*/.ts"] }

seemed to do the trick, so thanks for that solution. I don't really understand why this fixes the problem though!

JoaoP57 commented 2 years ago

Also having the same issue

image

this is failing for every type that we are importing, we don't use import type but we are having the same problem.

We do have have setup paths in tsconfig to improve our imports, @mypathDefinition , aside from that there is nothing else worth mentioning

Adding the "include": ["src/*/.ts"] to our "tsconfig.spec.json" did not help at all.

Any updates on this issue or any other way on how we can sort this out?

Angular 13.3.9 / jest 28.1.0 / types-jest 27.0.2 / jest-preset-angular 12.0.1

zotovamk commented 2 years ago

The same problem for Angular v14.2.1 / jest v28.1.3 / jest-preset-angular v12.2.2 The workaround didn't help

jreidgreer commented 2 years ago

I can replicate this problem on Angular 14, just by updating jest-preset-angular to 12.0.0 from 11.1.2.

However, if I set the resolutions section in package.json to point to ts-jest@^27.0.0, and then patch the files inside jest-preset-angular back to the old ts-jest import paths I can get it to work again. So for our repo at least, something changed in ts-jest@28 that caused type-only imports to no longer work.

Changing from import to import type does work, but simply adding "src/**/*.ts" to the tsconfig.spec.json does not.

barbados-clemens commented 2 years ago

So have been seeing reports of this for Nx workspaces.

here is a reproduce-able for an nx workspace that @jcabannes provided me, turning emitDecoratorMetadata: false is kind of a work around I've found, but not an ideal solution since I think it's just kicking the ball down the road.

https://github.com/nrwl/nx/issues/10660#issuecomment-1251030448

What we are seeing is basically the transformed code is missing an import being declared but is checking for that import in the decorator metadata of the compiled TS code. so removing the decorator metadata removes the check, causing the error to go away. Of course if the metadata is needed then tests will fail for other reasons, so bandaid solution for now. unsure if this is a jest-preset-angular/ts-jest/typescript/angular issue right now.

barbados-clemens commented 2 years ago

Just to add more to this. it looks like this is only happening when a decorator in involved. Almost like tsc emits incomplete info, such as not being able to transpile the import? this is why turning off emitDecoratorMetadata works for some, but isn't a full solution since tests can need that metadata.

Any more info anyone has come across?

philippesc commented 2 years ago

The same problem for Angular v14.2.1 / jest v28.1.3 / jest-preset-angular v12.2.2 The workaround didn't help

did you find any workaround?

philippesc commented 2 years ago

I am also having this kinda issue:

Screenshot 2022-10-23 at 15 11 23

We are using Typescript Paths to have readable import paths. We are not using Type-only imports.

We have Angular 14.2.5, Jest 28.1.3 and jest-preset-angular 12.2.2.

None of the above mentioned workarounds is working.

dylannnn commented 1 year ago

I'm also getting the same error: ReferenceError: lcl_routing_1 is not defined

Workaround does not help.

Angular v14.2.10 / jest v28.1.3 / ts-jest 28.0.8 / jest-preset-angular v12.2.2

anschm commented 1 year ago

Getting the same issues with jest-preset-angular v12.2.3.

anschm commented 1 year ago

It seems something is broken in ts-jest@28. Is their any bug opend at ts-jest?

ahnpnl commented 1 year ago

This is the limitation of current architecture when transforming files for Angular. Technically, when a Jest transformer does the work, we use TypeScript Language Service to transform Angular codes with some AST transformers in a scope of a Jest worker.

This bug is caused by the fact that, the Language Service is in a Jest worker doesn't have enough information to transform Angular codes which leads to this error. The way how Angular codes are processed is, it relies on a top level TypeScript Program which has all information.

To fix the problem problem, we need to follow how Angular does with their codes, which means we need to transform all the files even before Jest starts. When Jest starts, those transformed codes should be used and give it to Jest so Jest can run the tests.

anschm commented 1 year ago

@ahnpnl could you fix this problem or does anybody else doing this?

ahnpnl commented 1 year ago

Idk how to fix it yet:(

wimme commented 1 year ago

I also have the error with custom TypeScript decorators (only have this since migrating to Nx workspace 14.7.6 - Jest 28):

image

The class in which the error occurs:

import { DataMinerTypes, DMAObject } from '@cube-mobile/global';
import { IDMAGenericInterfaceQueryOption } from '../interfaces/QueryOption.interface';
import { DMAGenericInterfaceQueryOptionValues } from '../types/QueryOptionValue.type';
import { GIQueryOptionType } from '../types/QueryOption.type';
import { String as GI_String } from './Primitives';

@DataMinerTypes.registerType('DMAGenericInterfaceQueryOption')
export class DMAGenericInterfaceQueryOption<T extends DMAGenericInterfaceQueryOptionValues> implements IDMAGenericInterfaceQueryOption<T> {
    public Value: T;
    public MetaData: DMAObject[];

    public constructor(public ID: string, public Name: string, public IsMandatory: boolean, public Type: GIQueryOptionType) { }

    public getDisplayValue(): GI_String | GI_String[] {
        return this.Value ? new GI_String(this.Value.toString()) : null;
    }
}

The error is caused by the decorator: @DataMinerTypes.registerType('DMAGenericInterfaceQueryOption'):

@Injectable()
export class DataMinerTypes {
    public static registerType(typeId: string): Function {
        return <T extends new (...args: any[]) => object>(type: T) => {
            ...
        };
    }
}

Apart of the @Injectable() this is actually pure TypeScript, there's no Angular specific code in this.

The error disappears when putting emitDecoratorMetadata on false but this introduces other issues down the road as the decorator isn't properly executed.

n9niwas commented 1 year ago

for me this patch fixes the issue:

diff --git a/node_modules/jest-preset-angular/build/transformers/downlevel_decorators_transform/downlevel_decorators_transform.js b/node_modules/jest-preset-angular/build/transformers/downlevel_decorators_transform/downlevel_decorators_transform.js
index 7ee0201061e..c30b326b127 100644
--- a/node_modules/jest-preset-angular/build/transformers/downlevel_decorators_transform/downlevel_decorators_transform.js
+++ b/node_modules/jest-preset-angular/build/transformers/downlevel_decorators_transform/downlevel_decorators_transform.js
@@ -299,7 +299,7 @@ function getDownlevelDecoratorsTransform(typeChecker, host, diagnostics, isCore,
                 newMembers.push(createPropDecoratorsClassProperty(diagnostics, decoratedProperties));
             }
             const members = typescript_1.default.setTextRange(typescript_1.default.factory.createNodeArray(newMembers, classDecl.members.hasTrailingComma), classDecl.members);
-            return (0, ts_compatibility_1.createClassDeclaration)((0, ts_compatibility_1.combineModifiers)(decoratorsToKeep.size ? Array.from(decoratorsToKeep) : undefined, (0, ts_compatibility_1.getModifiers)(classDecl)), classDecl.name, classDecl.typeParameters, classDecl.heritageClauses, members);
+            return (0, ts_compatibility_1.updateClassDeclaration)(classDecl, (0, ts_compatibility_1.combineModifiers)(decoratorsToKeep.size ? Array.from(decoratorsToKeep) : undefined, (0, ts_compatibility_1.getModifiers)(classDecl)), classDecl.name, classDecl.typeParameters, classDecl.heritageClauses, members);
         }
         function decoratorDownlevelVisitor(node) {
             if (typescript_1.default.isClassDeclaration(node)) {

Idk for sure, but suspect that calling create* methods instead of update* makes TS program to forget information about whether a particular import is type-only or not.

ahnpnl commented 1 year ago

The transformer is downloaded from Angular source codes. If you wish to change it, you should consult with Angular team.

jbjhjm commented 1 year ago

The transformer is downloaded from Angular source codes. If you wish to change it, you should consult with Angular team.

@ahnpnl Can you elaborate on where that transformer comes from / how it is downloaded? I'll try to discuss this with angular team but it's hard to understand the actual mechanism. In downlevel-ctor.ts, getDownlevelDecoratorsTransform is imported from ./downlevel_decorators_transform which does not exist.

ahnpnl commented 1 year ago

You can check here https://github.com/thymikee/jest-preset-angular/blob/de881987c681d45907fb4f298704006813eb3912/scripts/prebuild.js#L8 it is a prebuild script that we first download and then build the package.

jbjhjm commented 1 year ago

@ahnpnl - over at angular repo, crisbeto says it would be ok to revert to use updateClassDeclaration as it seems the change to createClassDecoration was done without any particular reason.

However he/she states it would be good to add a unit test to ensure correct behavior in the future. I guess atm you know best about the differences caused by this and how they lead to reference errors...

Do you think you can provide some insights on what is wrong exactly? Think that will speed up the fix on angular side and make this work fine again :)

jbjhjm commented 1 year ago

Alright, crisbeto just freed up the way to make it work with ng15 compiler. Details: https://github.com/angular/angular/commit/33f35b04ef0f32f25624a6be59f8635675e3e131

So with the next ng minor release @ahnpnl should be able to make it work correctly again :)

jbjhjm commented 1 year ago

@ahnpnl - angular devs created a preview release (15.1.0-rc.0) which includes the mentioned fix just before the weekend. Can you release a preview too based on the compiler-cli@15.1.0-rc.0? Then we can all investigate if the issues are gone!

gmfun commented 1 year ago

@jbjhjm I tested this in angular 15.1 but its still not working

jbjhjm commented 1 year ago

@jbjhjm I tested this in angular 15.1 but its still not working

Thanks for the report but I'm only trying to manage communication between the devs... There's no new release of jest-preset-angular afaik so I'm wondering what you tested with?

anschm commented 1 year ago

We need a new release of jest-preset-angular which points to angular 15.1.0.

jbjhjm commented 1 year ago

@anschm @gmfun a new version has just been released, happy testing! Will check it out myself tomorrow .

jbjhjm commented 1 year ago

Perfect, 12.2.4 seems to be fully compatible with ts 4.8! Updated my issue repo and all tests are passing as expected now. ( https://github.com/jbjhjm/jest28-bug ) Unit tests in our main repo (ng14, jest28, ts4.8) are passing fine too. I think this issue is resolved @ahnpnl !

anschm commented 1 year ago

I checked this today in my project and its finally working. I think this Bug could be closed.

BaHXeLiSiHg commented 1 year ago

Angular 15.1.0, Nx Workspace 15.2.4, Jest 28.1.3, Jest Preset Angular 12.2.4, TypeScript 4.8.2 - getting errors on services/classes imports: Cannot find module 'service_path' from 'test_file_path'. On previously working tests (before Angular 15 and other updates).

image

Somebody else facing something like this?

jbjhjm commented 1 year ago

Sounds like a different issue to me as you are importing a class, not types. nx 15.2.4 is kinda outdated, would be smart to update that prior to any debugging attempts. If the issue doesn't disappear, verify if jest uses correct tsconfig that knows how to resolve 'common/services'.

marleypowell commented 12 months ago

I have reproduced this issue with a fresh NX setup for an angular application. https://github.com/marleypowell/nx-jest-repro/tree/master/nx-jest-repro

Dependency injection in the constructor doesn't work with type only imports. As soon as I remove the type only import this works again.

image image

Something is different with constructor dependency injection as the static method decorator is fine image

jbjhjm commented 12 months ago

@marleypowell hmm this WAS fixed for sure. This unit test is responsible for ensuring it works: https://github.com/angular/angular/blob/17.0.x/packages/compiler-cli/test/downlevel_decorators_transform_spec.ts#L703

Unfortunately my projects are still months away from ng17 so I can't reproduce or help out with this.

After all this was a issue on angular side, so as you already have a reproduction setup you may want to open a regression issue over there.

marleypowell commented 12 months ago

Something else I found is that using a type alias fixes the problem too. image

It also happens without any decorators on the constructor image

times29 commented 8 months ago

Something else I found is that using a type alias fixes the problem too. image

It also happens without any decorators on the constructor image

This works!!! Thanks so much!!!

joepvl commented 5 months ago

Something else I found is that using a type alias fixes the problem too. image

It also happens without any decorators on the constructor image

Great find, thanks! Seems like you do need to use the alias though, so it'd be constructor(public test: InterfaceTypeAlias) {}. At least that's what made it work for me with Angular 17.x + jest-preset-angular 14.0.0.