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.

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

tests failing on angular 9 after running ngcc with @angular/localize #347

Closed
pkaufi opened this issue Feb 11, 2020 · 21 comments
Closed

tests failing on angular 9 after running ngcc with @angular/localize #347

pkaufi opened this issue Feb 11, 2020 · 21 comments
Labels
ivy Ivy compatible Not An Issue Not jest-preset-angular issue

Comments

@pkaufi
Copy link

pkaufi commented Feb 11, 2020

Describe the Bug

After running ngcc as proposed in the angular update guide (https://update.angular.io/#8.2:9.0), tests are failing with the error message

It looks like your application or one of its dependencies is using i18n.
Angular 9 introduced a global `$localize()` function that needs to be loaded.
Please run `ng add @angular/localize` from the Angular CLI.
(For non-CLI projects, add `import '@angular/localize/init';` to your `polyfills.ts` file.
For server-side rendering applications add the import to your `main.server.ts` file.)

I added a small reproduction below (updated and adapted the example app in this repository), where I installed @angular/localized with the help of the cli (ng add @angular/localized). It then added an import to the polyfills.ts files automatically (polyfills.ts).
After I investigated a bit, I found out, that polyfills.ts is not picked up during the test run.
When you add either import './polyfills'; or import '@angular/localize/init'; to setupJest.ts, the tests run successfully. But isn't that what we have the tsconfig's for? In tsconfig.spec.json for example, you can see that we have

"files": [
    "src/polyfills.ts"
  ]

What seems strange to me is and what I don't understand is, that before you run ngcc, the tests run successfully, even though polyfills.ts is not picked up. However, I also don't know exactly what ngcc is doing.

Minimal Reproduction

https://github.com/pkaufi/ngcc-i18n-jest/tree/jest-preset-angular

Expected Behavior

Tests should run successfully after running ngcc

Environment

see reproduction repository

@wtho
Copy link
Collaborator

wtho commented Feb 11, 2020

Hey!
Thanks for reporting and providing a repro!

The ngcc (angular compatibility compiler) basically upgrades modules, that have not been compiled with the ivy compiler (ngtsc), to be compatible with it. In the README:.

This compiler will convert node_modules compiled with ngc, into node_modules which appear to have been compiled with ngtsc.

There are currently four types of code that have to be transformed, although the migrations itself are not defined inside the ngcc package (link to transformation calls):

  • Switch Marker
  • Decorators
  • Modules with providers
  • Private declarations

I personally do not have time to look into it before March, but I just had an idea. Maybe you can try to move the import to setupJest?
Probably it won't work though, as there are probably have to happen transformations during the compilation.

I'll have a look at it in two weeks.

@sergeik
Copy link

sergeik commented Feb 11, 2020

This temporary solution works for me:
jest.config.js

module.exports = {
    name: 'module-report',
    ...
    setupFiles: ['./setup-jest.ts']
};

setup-jest.js

import '@angular/localize/init';

@pkaufi
Copy link
Author

pkaufi commented Feb 12, 2020

This temporary solution works for me:
jest.config.js

module.exports = {
    name: 'module-report',
    ...
    setupFiles: ['./setup-jest.ts']
};

setup-jest.js

import '@angular/localize/init';

I personally do not have time to look into it before March, but I just had an idea. Maybe you can try to move the import to setupJest?

Exactly, that's what I meant with

When you add either import './polyfills'; or import '@angular/localize/init'; to setupJest.ts, the tests run successfully.

It will definitely work like this and shouldn't be a blocker for upgrading to angular 9.

@wtho thank you for having a look at it at providing some more insight into ngcc
Can you maybe provide some more information, if the exclusion of the polyfills.ts is an intentional one or not? If you run the tests with karma-jasmine, polyfills.ts declared in tsconfig.spec.json are picked up properly

@skorunka
Copy link

skorunka commented Feb 15, 2020

Add import '@angular/localize/init'; to test-setup.js file. I think jest-preset-angular could do it instead. Unfortunately you have to do it for every angular library (for every test-setup.ts file). Or any idea where to put it and make it working for all tests in angular workspace?

@sergeik
Copy link

sergeik commented Feb 15, 2020

... any idea where to put it and make it working for all tests in angular workspace?

In my project I've added this snipet code into the jest.config.js file located in the root of monorepository. Other projects use this root jest.config.js file as preset:

Root jest.config.js

const path = require('path');
const rootSetupFile = path.resolve('./setup-jest.ts');
module.exports = {
    ...
    setupFiles: [rootSetupFile],
};

Root setup-jest.ts

import '@angular/localize/init';

Project jest.config.js

module.exports = {
    name: 'module-authentication',
    preset: '../../../jest.config.js',
    ...
};

@wtho
Copy link
Collaborator

wtho commented Feb 15, 2020

If the demand is high I would argue we should add it to jest-preset-angular itself.
But I do not know how many projects use i18n.
I really like @sergeik's approach for workspace-based projects!

if the exclusion of the polyfills.ts is an intentional one or not?

We are just passing tsconfig.spec.json to ts-jest.

Unfortunately ts-jest has an undocumented "feature" to only respect compilerOptions and fileNames from tsconfig.json.
Although I do not know what fileNames in tsconfig is for, maybe this can be fixed by changing it to files, I guess this is a typo.

Anyways I think many imports into polyfills.ts like webanimations-js should not be included in every unit test. Remember that each unit test in jest has its own runtime, and Jest shines the most when the setup is as minimal as possible. Especially if you setup a bloated polyfills.ts for IE legacy support, it should not be imported to the unit tests!

@just-jeb
Copy link

just-jeb commented Feb 18, 2020

I think the problem is bigger than just projects that use i18n.
Looks like having a custom transformer that uses ngtools/webpack is unavoidable, otherwise this preset is prone to errors every time Angular team changes something in the ngcc processor.
More details here.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 18, 2020

Although I do not know what fileNames in tsconfig is for, maybe this can be fixed by changing it to files, I guess this is a typo.

No it's not typo, fileNames is a property of ParsedCommandLine typescript interface. This interface represents for tsconfig.json when it is parsed into json object. fileNames will represent the array value of files you specify in tsconfig.json

@wtho
Copy link
Collaborator

wtho commented Feb 18, 2020

it's not typo

Ok, thanks for clarifying!
This means the files array should be passed to ts-jest, and therefore to tsc.
Seems like more research has to be done to understand why they do not get loaded.

having a custom transformer that uses ngtools/webpack is unavoidable

I would love to use it, but unfortunately ngtools/webpack relies on the tsc compile typechecker and program, which ts-jest does currently avoid. Using it in ts-jest would require a major refactoring in the ts-jest compilation process, see kulshekhar/ts-jest#1146 for more information.
Also this means enabling babel as an alternative compiler will not be possible anymore, which we are about to achieve for the Angular 8 scenario (#317).

I feel like this discussion is key for the future of this preset, but on the other hand I am also confident Angular will not modify the ngcc processor very often.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 18, 2020

At the moment I have a branch on ts-jest which includes compiler by Program. However I need some helps to clarify and test if it is the correct implementation. You can find it here

@wtho
Copy link
Collaborator

wtho commented Feb 18, 2020

Ok! I will be free-ish from next week on, will definitely get into this then!

@wachri
Copy link

wachri commented Feb 25, 2020

I found another problem after running ngcc:

PrettyFormatPluginError: Cannot read property 'element' of undefinedTypeError: Cannot read property 'element' of undefined

      19 |
      20 |   it(`should match snapshot`, () => {
    > 21 |     expect(fixture).toMatchSnapshot();
         |                     ^
      22 |   });
      23 | });
      24 |

      at Object.print (node_modules/jest-preset-angular/build/AngularSnapshotSerializer.js:21:49)
      at it (src/app/match-test/match-test.component.spec.ts:21:21)

Created also a reproduction repository:
https://github.com/wachri/jest-preset-angular-9-ngcc-bug

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 25, 2020

I found another problem after running ngcc:

PrettyFormatPluginError: Cannot read property 'element' of undefinedTypeError: Cannot read property 'element' of undefined

      19 |
      20 |   it(`should match snapshot`, () => {
    > 21 |     expect(fixture).toMatchSnapshot();
         |                     ^
      22 |   });
      23 | });
      24 |

      at Object.print (node_modules/jest-preset-angular/build/AngularSnapshotSerializer.js:21:49)
      at it (src/app/match-test/match-test.component.spec.ts:21:21)

Created also a reproduction repository:
https://github.com/wachri/jest-preset-angular-9-ngcc-bug

seems to be a problem in AngularSnapshotSerializer.ts

@wachri
Copy link

wachri commented Feb 25, 2020

seems to be a problem in AngularSnapshotSerializer.ts

Should i open another ticket?

I debugged it a little bit and for me it looks like that the old ViewEngine is used instead of ivy.
Before the running ngcc var componentName = val.componentRef._elDef.element.name is available (https://github.com/thymikee/jest-preset-angular/blob/master/src/AngularSnapshotSerializer.js#L26)

When i do console.warn(val.componentRef); it looks like:

ComponentRef_ {
      _view:
       { def:
          { factory: [Function],
            nodeFlags: 33669121,
            rootNodeFlags: 33554433,
            nodeMatchedQueries: 0,
            flags: 0,
            nodes: [Array],
            updateDirectives: [Function],
            updateRenderer: [Function: NOOP],
            handleEvent: [Function: handleEvent],
            bindingCount: 0,
            outputCount: 0,
            lastRenderRootNode: [Object] },
//...

After running ngcc it looks like

ComponentRef {
      location:
       ElementRef {
         nativeElement:
          HTMLDivElement {
            __ngContext__: [LRootView],
            __ng_debug__: [DebugElement__POST_R3__] } },
      _rootLView:
       LRootView [
         null,
         TView {
           type: 0,
           id: -1,
           blueprint: [LViewBlueprint],
           template: null,
           queries: null,
           viewQuery: null,
           node: [TNode],
//...

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 25, 2020

Ya I think it seems to be not related to this issue. My guess is the current serializers run with VE AST only but not Ivy AST. Is it right @wtho ?

@wtho
Copy link
Collaborator

wtho commented Feb 25, 2020

Yeah, internals have changed. This should be easily solvable by making the serializers compatible with both.
I think an own issue is a good idea.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Apr 2, 2020

FYI: ts-jest25.3.0 (has some downgrade in performance but will be fixed in 25.3.1) released which includes possibility to access internal ts Program created by ts-jest for any AST transformers.

@Godrules500
Copy link

Any updates on this?

@wtho
Copy link
Collaborator

wtho commented Oct 5, 2020

Ivy does quite some magic under the hood, as it is a compiler.
Instead of tackling all issues with ivy directly by implementing workarounds (which might become quite a lot the more people use jest + angular in edge cases), we decided to use the Angular Compiler itself wrapped in a jest transformer.

It is still a long way to go, but we are on the way (in our free time, unpaid). Shout-out to @ahnpnl for the ton of work he is currently putting in.

Subscribe #409 for more updates.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Oct 5, 2020

Hi @Godrules500 , if you are having issues with @angular/localize, please use this workaround:

This temporary solution works for me:
jest.config.js

module.exports = {
    name: 'module-report',
    ...
    setupFiles: ['./setup-jest.ts']
};

setup-jest.js

import '@angular/localize/init';

@ahnpnl ahnpnl changed the title tests failing on angular 9 after running ngcc tests failing on angular 9 after running ngcc with @angular/localize Oct 5, 2020
@ahnpnl ahnpnl added the 🐛 Bug label Oct 5, 2020
@ahnpnl
Copy link
Collaborator

ahnpnl commented Oct 24, 2020

Due to the fact that testing with jest is different from Angular CLI karma + jasmine so any files which are required for test environment should be included in your setup jest file. On contrary, Angular CLI actually loads assets with webpack and browser will load all files including files like polyfills.ts

We can solve this difference by using AST transformer to inject polyfills, angular localized import or any global files into your setup jest, but this comes with a cost on performance (walking in AST tree is not free).

I would propose that adding polyfills.ts or import Angular localize in your test file is the best solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ivy Ivy compatible Not An Issue Not jest-preset-angular issue
Projects
None yet
Development

No branches or pull requests

8 participants