InlineHtmlStripStylesTransformer is not strict enough

[FrozenPandaz]

Current Behavior

Any object literals have their property assignments transformed thus causing source code such as:

console.log({
  styles: ['styles.css']
})

to print out

{
  styles: []
}

Expected Behavior

Only object literals passed to @angular/core's @Component decorator should have it's property assignments transformed.

[FrozenPandaz]

Unless someone else wants to tackle this, I can try and make a PR.

[ahnpnl]

PR is welcome 👍😀

[wtho]

Yeah, this caveat is known and discussed

• in the transformer itself

  jest-preset-angular/src/InlineHtmlStripStylesTransformer.ts

  Lines 10 to 15 in 075498e

  	*
  	* The assignments can be located anywhere in a file.
  	* Caveats:
  	* All properties 'templateUrl', 'styles', 'styleUrls' ANYWHERE will be modified, even if they
  	* are not used in the context of an Angular Component.
  	*

  )
• the original PR
• this follow-up PR, caused by
• this issue.

Problem is, that theoretically someone might rename the annotation, or pull an object into the annotation, that is not declared inside the annotation itself.

The AST transformer does not know if an object is actually the Angular Component Decorator or if an object with styles and templateUrl is actually a component configuration, it only knows in what relation code pieces are and how they are named.

Consider these examples:

import { Component } from '@angular/core';


// classic and easy
@Component({ templateUrl: './file.html' }) export class MyFirstComp { }

// difficult
const MyAnnotation = Component;
@MyAnnotation({ templateUrl: './file.html' }) export class MySecondComp { }

// difficult
const componentConfig = { styles: [], templateUrl: './file.html' };
@Component(componentConfig) export class MyThirdComp { }

// difficult
const partialComponentConfig = { templateUrl: './file.html' };
@Component({...partialComponentConfig, styles: []}) export class MyThirdComp { }

We should discuss approaches and their caveats before implementing it.

---

Edit: fixed linked issues

[electrichead]

Would it be an expectation to even support the second case?

const MyAnnotation = Component;
@MyAnnotation({ templateUrl: './file.html' }) export class MySecondComp { }

For the others we are guaranteed to be in @Component.

I guess we are also looking at this scenario from the linked issue:

Yeah, good catch!

The astTransformer modifies the component when it is loaded by jest through ts-jest. When the metadata is overridden in alert-follow.component.spec, it already is the transformed and transpiled version with template. If you then set templateUrl: '<path-to-mock>', it will be added next to template in the metadata, instead of overriding it. That leads to the compiler complaints about both assignments being defined.

Possible solutions

• Ask the users of this preset to only set template: require(<path-to-mock>]) in tests
• let the transformer also process .spec files that set templateUrl (I guess the regex used before also transformed tests)

Basically we could not only look inside decorators, but transform any assignment named templateUrl to template + require. I am not sure if there might be other assignments in Angular projects that would use templateUrl in a different context.

[wtho]

I agree, the second example is not very important.

The third and fourth are the problematic ones.

@electrichead The AST transformer is written in TypeScript, but it works with AST objects. AST is a representation of the code text, not of the evaluated objects.

So this situation

const componentConfig = { styles: [], templateUrl: './file.html' };

@Component(componentConfig) export class MyThirdComp { }

is definitely a problem, as in this example each line is an independent object. References are made when JavaScript is interpreted, not when the AST transformer is executed. The styles assignment is not inside the decorator and therefore we would have to search for the componentConfig variable in the whole file, maybe even in other files if it is imported.

[FrozenPandaz]

@wtho I see the issues. That is fair.

Let's make it not as strict as looking for where the object literal is.

I think if we check if the file imports Component from @angular/core it will be safe enough. The difficult cases are still satisfied since they all have to import from @angular/core or @angular/core/testing.

Then we can exclude files which do not include:

import { Component } from '@angular/core';
import * as core from '@angular/core';
import { Component as Comp } from '@angular/core';

And same for @angular/core/testing?

import { TestBed } from '@angular/core/testing';
import * as testing from '@angular/core/testing';
import { TestBed as Bed } from '@angular/core/testing';

I suppose they can import the options from somewhere else.. but I feel like that's a better Caveat to have.

What do you think?

[wtho]

Ok, so one more thought I just had, templateUrl is actually more important to be transformed than styles to be removed. Without templateUrl being transformed to template, it will not even load.

styles on the other hand is just removed for performance issues (correct me if I'm wrong), as we assume styles are usually not tested in jest, that is better done in e2e frameworks.

Proposal

We could also think about handling them differently, although this might complicate the code unnecessarily.

templateUrl:

• replaced everywhere
• alternatively only (as suggested by @FrozenPandaz) in files where Component gets imported from @angular/core or TestBed gets imported from @angular/core/testing

styles:

• replaced only if declared directly inside component decorators

This would seem more practical to me as styles is also used in more contexts than templateUrl.

[ahnpnl]

I think the proposal looks practical and good. I vote for it.

[willyboy]

If someone lands here before that PR is merged - I was able to hack around the problem temporarily by changing:

mockObj = {
    styles: 'a {color: blue}'
}

to

mockObj = {}
mockObj.styles = 'a {color: blue}'

[ali-kamalizade-leanix]

This error was driving me mad until I found this issue.
Glad to see a PR is on the way

[dirkluijk]

Wow, discovered this today as well. Suddenly a unit test broke after upgrading to Jest 24.

[wtho]

Yeah, I am almost done with the PR, will submit it this weekend.

It's not really an error, it is an intrusive caveat that kept the transformer simple, but can create side-effects.

[dirkluijk]

I renamed my property as an easy workaround. 😉

[SLKnutson]

This is a nightmare to debug. We were lucky to find it by searching thought node_modules. Is there any that you can do this without using AST Transformations? Or if you are going to use AST Transformations, only modify it when you are sure that it is appropriate?

[wtho]

@SLKnutson The PR is on the way already #261

If you want to avoid them, you can remove AST Transformers by overriding the array in your jest config and clearing it. To still use jest without the transformers, you will have to inline your templates and styles though.

Also you can test the branch of the PR with the appropriate implementation on wtho:feat/ast-transformer-strict-styles in your project using (in your package.json):
"jest-preset-angular": "github:wtho/jest-preset-angular#feat/ast-transformer-strict-styles"

Testing and reviewing the implementation can speed up the merge.

Finally you are free to implement and use custom transformers.