feat: alternative setup with babel/typescript #317

wtho · 2019-10-02T21:46:20Z

This would add an alternative compiling method alongside ts-jest.

Done so far

Babel config is in plain js inside new babel folder
second preset is also located in there
added second preset inside babel folder, which can be used via preset: 'jest-preset-angular/babel' in jest config
added basic preconfigured babel.config.js
added load-html-transformer.js to deal with html files
added inline-template.plugin.js to do what InlineFilesTransformer.ts does
removed ts-jest from dependencies, which means it would have to be be installed when using this preset with ts-jest from now on

The following libraries have to be installed alongside the project, when setting up jest-preset-angular + babel

@babel/core
@babel/preset-env
@babel/preset-typescript
@babel/plugin-proposal-decorators
@babel/plugin-proposal-class-properties
babel-plugin-transform-typescript-metadata
babel-plugin-const-enum

This would be especially interesting regarding some feedback from the community about speed difference in bigger projects. Babel does no TypeChecking in the .test.ts/.spec.ts files itself, but going through the issues of ts-jest (e. g. kulshekhar/ts-jest#1115, surely ahnpnl knows more) some projects are falling back to isolatedModules: true already, which is also skipping the type checking. Furthermore type checks can still be achieved separately using tsc --noEmit.

Personally I think we need some more restructuring in the preset to keep ts-jest and babel separated as two exclusive usage options, so this might take some time. I think this can land some time later this year.

TODOs

Write tests for babel inline template plugin
Figure out preset folder setup with ts-jest and babel separated (see discussion in comments below)
Run example app for both, babel and ts-jest (see discussion)
Make PluginInlineAngularTemplate to also remove styleUrls to behave like the ts-jest transformer
Babel and ts-jest Usage Documentation in README.md
Quick user guidance to decide between the two presets

Closes #308

Edit1: updated list of babel dependencies
Edit2: added TODO list
Edit3: update TODOs

ahnpnl · 2019-10-03T05:49:08Z

I think we need also plugin for babel plugin const enum for this PR

wtho · 2019-10-03T10:57:39Z

Yeah! Let's add a test with enums in the example app!

wtho · 2019-10-16T15:38:17Z

In last force push:

Added a test with enum and const enum and added the babel plugin for const enums to ensure the preset can deal with this TS language feature when using babel.

wtho · 2019-10-22T20:49:18Z

In the newest push I added some opiniated changes, so feel free to correct it wherever it looks seldom.

Proposal: Folder Structure with babel & ts-jest

.
├── README.md
├── package.json
├── [other-base-level-files]
└── src
    ├── AngularNoNgAttributesSnapshotSerializer.js
    ├── AngularSnapshotSerializer.js
    ├── HTMLCommentSerializer.js
    ├── index.ts
    ├── reflectMetadata.ts
    ├── setupJest.ts
    ├── babel
    │   ├── babel.config.ts
    │   ├── jest-preset.ts
    │   ├── load-html-transformer.ts
    │   └── plugin-inline-angular-template.ts
    ├── ts-jest
    │   ├── InlineFilesTransformer.ts
    │   ├── jest-preset.ts
    │   ├── StripStylesTransformer.ts
    │   └── TransformUtils.ts
    ├── __tests__ [folder with tests]
    └── zone-patch [folder with sources]

ts-jest and babel would be built to jest-preset-angular/build/ts-jest and jest-preset-angular/build/babel respectively.

This would lead for users to adjust their jest.config.js:

module.exports = {
  preset: "jest-preset-angular/build/ts-jest",
  // or
  preset: "jest-preset-angular/build/babel"
}

We could add additional folders with a proxy jest-preset.js at jest-preset-angular/babel and jest-preset-angular/ts-jest, but that would add additional confusion to our project itself. Anyway there should be a jest-preset.js in the root that points at build/ts-jest/jest-preset.js for backwards compatibility.

Proposal: Run project example app with ts-jest and babel
To run the project with both the settings, I suggest we set the preset value in example/jest.config.js programmatically, e. g. depending on a env variable:
export JPA_TEST_MODE=ts-jest yarn test

// jest.config.js
const testMode = process.env.JPA_TEST_MODE || 'ts-jest';

module.exports = {
  preset: `jest-preset-angular/build/${testMode}`,
  ...

This way we do not have to duplicate any code base and make sure the two transpilation modes are actually easily exchangable. Both transpilation plugins from babel and ts-jest have to be installed initially, which would also speed up the CI test time.

There is one issue (see CI tests): tsc transpiles undefined class properties differently than babel/typescript does (babel adds the property):

  <app-calc
    hasAClass="false"
    image={[Function String]}
+   observable$="undefined"
    prop1={[Function Number]}
  >

We have to plugin optional tests somehow. Any experience with this?

This PR would be a big difference for this preset, so it is quite important to me what you two think, @ahnpnl and @thymikee. Note that the speedup of the example app testrun is similar to the one of ts-jest with isolatedModules: true (~4s). Would be interested if there is any difference in bigger projects though!

ahnpnl · 2019-10-23T05:45:17Z

I like the idea of having 2 internal presets for babel and ts-jest and let users choose what they want to use.
Regarding to the folder structure, all LGTM but imo I like putting tests outside src folder. It will be easier for us to build and release (less configuration in excluding something).

I haven't tried to use babel yet so I wasn't aware of the different in compiling. Looks interesting. Is it intended behavior of babel team ?

wtho · 2019-10-23T13:39:06Z

I think the different compilation output is a minor design decision. All the plugins are created by different people, even the "official" babel ones, and babel and typescript clearly do not work too closely together. It behaves the same when running the code, and that's the important part.

class TestComponent {
  field: boolean;
}

compile output using tsc (swallows the undefined field)

var ClassComponent = /** @class */ (function () {
    function ClassComponent() {
    }
    return ClassComponent;
}());

compile output using babel (adds the undefined field)

var ClassComponent = function ClassComponent() {
  _classCallCheck(this, ClassComponent);
  this.field = void 0;
};

I am not so sure about the __tests__ being outside of src, it was there since we added the build folder approach (#307) and it is just a single exclude in tsconfig.json on the __tests__ folder. The tests will never be compiled to build and src anyway will never be included in the release, it's only the build folder.

wtho · 2019-10-25T20:49:11Z

Now it is definitely ready for review, although I do not feel good about the test mode detection. Maybe you have a better idea?

Also with this test that behaves differently (observable$="undefined") I am not sure if my inline snapshot is a good solution.

Good thing is I used the very same test cases for the babel ast transformer as for the ts-jest ast transformer. I duplicated them for now, but can extract them to a single source.

ahnpnl · 2019-10-27T10:08:42Z

Now it is definitely ready for review, although I do not feel good about the test mode detection. Maybe you have a better idea?

I'd do similar things as well.

Also with this test that behaves differently (observable$="undefined") I am not sure if my inline snapshot is a good solution.

Maybe use fixture.nativeElement instead of fixture. I think testing HTML template shouldn't use that fixture but rather than HTML elements rendering. And for the different output related to ts-jest vs babel compiling, I think there should be a note highlighted in the documentation about that.

package.json

wtho · 2019-10-27T14:29:52Z

Maybe use fixture.nativeElement instead of fixture. I think testing HTML template shouldn't use that fixture but rather than HTML elements rendering.

Snapshotting fixture.nativeElement does solve the issue, as it does not render the properties, but I guess that was the point of the original test. The test then will diff this:

- <app-calc
-   hasAClass="false"
-   image={[Function String]}
-   prop1={[Function Number]}
+ <div
+   id="root0"
  >
    <p
      class="a-default-class"
    >
       calc works! 1337 another text node test-image-stub 
    </p>
- </app-calc>
+ </div>

I am not so sure if that's a good snapshot anymore.

ahnpnl · 2019-10-28T16:05:15Z

The snapshot thing indeed depends on the purpose of users when testing a component. If users care about the property rendered by ts file, fixture is the right one. Otherwise if users only care about html change, fixture.nativeElement is the choice.

I think we don't need to care about this too much. Perhaps we can add a note in README to warn about the difference of snapshot relative to usage of tsc vs babel. I think that should be fine.

At work I usually use fixture.nativeElement because I only want to check about html change.

wtho · 2019-10-28T16:48:29Z

👍 Already added a small note in the README.md. I know, it's not very visible, but someone who does read through the README will encounter it.

BensonBen · 2019-12-02T15:40:27Z

@thymikee normally, I don't comment on pull requests. But I'm pretty interested in using this if it leads to test suite speed increases, so your feedback is valued.

For now, I'm actually going to be converting my teams jest tests to use babel. As my initial testing lead to some serious speed improvements with ~400 individual tests.

@wtho I can report rough performance improvements if you'd like. Also, I'm using the isolatedModules: true option in our tsconfig.

thymikee · 2019-12-02T15:42:17Z

Making it easier to use Babel sounds like a good idea, I just didn't find the time to have a look yet :D

src/babel/load-html-transformer.ts

BREAKING CHANGE: add an alternative preset for babel and restructures the whole repository to treat babel and ts-jest equally. Furthermore `ts-jest` is not a dependency of the preset anymore. The jest config and package.json of users will have to be adjusted, see the CHANGELOG.md for migration information. Closes thymikee#308

wtho · 2019-12-02T18:58:09Z

@BensonBen Yeah, a performance evaluation would be super helpful! I do not have a big codebase at hand currenlty. Maybe you can install jpa using npm install -D git://github.com/wtho/jest-preset-angular.git#feat/babel-jest-angular and run them with the new configuration.

Also I'm interested in which adjustments were required to use babel. Making switching as easy as possible would be desired from our side.

thymikee

This looks mostly good and I like the idea. I wouldn't rush so hard with such a breaking change though. Let's give the community some time to actually test Babel preset without breaking anything. IMHO, we can leave jest-preset-angular to point to the build/ts-jest and in the readme and release notes encourage people to try out Babel because of speed or something.

So, things to address:

make it a non-breaking change (no point in breaking with an independent feature, isn't it?)
keep ts-jest as dependency for now
consider adding a deprecation notice in the next major

wtho · 2019-12-02T23:48:31Z

True, we can keep the structure and just point it at ts-jest as default. I just liked the equality between the two options and disregarded backwards-compatibility.

I would also suggest to create a new issue to gather babel feedback and link the issue next to the babel setup notes.

Changes in last commit

add default jest-preset.js to provide full backwards-compatibility
add ts-jest to dependencies
Set package version to 8.1.0 (ready for squash, tag & push)
adjust CHANGELOG.md
rephrase README.md, add notes for preset comparison feedback (can be removed later again)

viceice · 2019-12-03T07:27:23Z

I would like to test, but i get errors with babel

ts-jest

babel
I get errors like: Can't resolve all parameters for ...: (?).

babel is a lot slower too, using following additional settings for ts-jest

diagnostics: false,
isolatedModules: true,

wtho · 2019-12-03T07:51:39Z

Interesting!

Can you create a gist just containing the classes that have injection issues (and the classes/providers that could not be injected)?
Decorators, classname and constructor signatures are sufficient. Methods, class fields and templates are not involved, so better remove them.

This information would help a ton!

viceice · 2019-12-03T08:26:51Z

looks like it is always the same class MyHandler which is used to provide a service.
I hope this is enough to reproduce.

Technically I'm using a nrwl workspace with multiple library packages.

import { NgModule, Injectable, Optional } from '@angular/core';
// import { NGXLogger } from 'ngx-logger';
// import { MessageService } from 'primeng/api';
import {
    MissingTranslationHandler,
    MissingTranslationHandlerParams,
    TranslateCompiler,
    TranslateLoader,
    TranslateModule,
    TranslateService,
} from '@ngx-translate/core';

@Injectable({ providedIn: 'root' })
export class Logger {
  constructor(/* private readonly _log: NGXLogger */) {}
}

@Injectable()
export class MessageService {
}

class MyHandler {
  constructor(@Optional() private messageService: MessageService, private readonly _log: Logger) {}
  handle(params: MissingTranslationHandlerParams) { return params.key; }
}

@NgModule({
  imports: [ TranslateModule ],
})
export class SharedModule {
constructor(ts: TranslateService, private readonly _log: Logger) {}
}

describe('SharedModule', () => {

	beforeEach(async(() => {
		TestBed.configureTestingModule({
		  imports: [
			TranslateModule.forRoot({
			  missingTranslationHandler: { provide: MissingTranslationHandler, useClass: MyHandler },
			  useDefaultLang: false,
			}),
		  ],
		  providers: [{ provide: MessageService, useValue: { add: jest.fn() } }],
		}).compileComponents();
	});
	
	it('should create', () => {
        expect(SharedModule).toBeDefined();
    });
});

thymikee · 2019-12-03T09:14:24Z

CHANGELOG.md

+* Add preset `babel` as transpilation alternative to `ts-jest` ([#317](https://github.com/thymikee/jest-preset-angular/pull/317)).
+
+#### Migration Guide
+* The preset reference in the jest config has to be updated from `"jest-preset-angular"` to `"jest-preset-angular/build/ts-jest"`. To use babel, replace `ts-jest` with `babel`. Furthermore, when using `ts-jest`, it has to be added as `devDependency` to the project, or `babel` packages alternatively (see README.md).


This is not relevant now

thymikee · 2019-12-03T09:19:12Z

CHANGELOG.md

+### v8.1.0
+
+#### Features
+* Add preset `babel` as transpilation alternative to `ts-jest` ([#317](https://github.com/thymikee/jest-preset-angular/pull/317)).


Suggested change

* Add preset `babel` as transpilation alternative to `ts-jest` ([#317](https://github.com/thymikee/jest-preset-angular/pull/317)).

* Expose a new preset `jest-preset-angular/build/babel` for transforming files with Babel instead of `ts-jest` ([#317](https://github.com/thymikee/jest-preset-angular/pull/317)).

Side note: would be nice if we could get rid of build part, just jest-preset-angular/babel – easy enough with re-exporting from top-level.

I agree. Maybe we can just publish the build folder with the package.json? Have to check out how other projects solve this.

thymikee · 2019-12-03T09:20:24Z

README.md

@@ -19,6 +22,19 @@ npm install -D jest jest-preset-angular @types/jest

 This will install `jest`, `@types/jest`, `ts-jest` as dependencies needed to run with Angular projects.

+Additionally you need `babel` packages if you want to use `babel`.


Please remove, no need to mention it here

Suggested change

Additionally you need `babel` packages if you want to use `babel`.

thymikee · 2019-12-03T09:20:39Z

README.md

@@ -19,6 +22,19 @@ npm install -D jest jest-preset-angular @types/jest

 This will install `jest`, `@types/jest`, `ts-jest` as dependencies needed to run with Angular projects.

+Additionally you need `babel` packages if you want to use `babel`.
+
+### babel


How about:

Suggested change

### babel

### jest-preset-angular/babel

thymikee · 2019-12-03T09:21:14Z

README.md

+Additionally you need `babel` packages if you want to use `babel`.
+
+### babel
+`babel` uses the babel compiler to generate JavaScript, without type-checking your test files before doing so. This might result in a different test run performance. You can still run type-checking using `tsc --noEmit`. For additional TypeScript-supported language features you might have to install more babel packages. Note that `babel` also can differ slightly from `tsc`, e. g. in compiling a class to a function.


Suggested change

`babel` uses the babel compiler to generate JavaScript, without type-checking your test files before doing so. This might result in a different test run performance. You can still run type-checking using `tsc --noEmit`. For additional TypeScript-supported language features you might have to install more babel packages. Note that `babel` also can differ slightly from `tsc`, e. g. in compiling a class to a function.

`babel` preset uses the Babel compiler to generate JavaScript without type-checking your test files before doing so. This might result in a different test run performance. You can still run type-checking using `tsc --noEmit`. For additional TypeScript-supported language features you might have to install more babel packages. Note that `babel` also can differ slightly from `tsc`, e.g. in compiling a class to a function.

wtho · 2019-12-03T22:44:27Z

@viceice I tried out the tests and as soon as I add @Injectable to MyHandler it works in babel, but not in ts-jest, so this seems contradictary to what you said.

Maybe we can figure it out in this minimal repro: wtho/jpa-babel-bug

viceice · 2019-12-04T13:49:53Z

@wtho Next bug wtho/jpa-babel-bug#2

viceice · 2019-12-04T14:56:39Z

found workarounds for the bugs, here are the new timings:

second run

wtho · 2019-12-20T16:03:46Z

While we have to wait for leonardfactory/babel-plugin-transform-typescript-metadata#24, you guys can help us by doing further tests on bigger codebases.

Please try out the babel preset using a branch where I included the babel-plugin PR and the built js files (which would not be included normally checking out a git branch as npm dep).

npm i -D jest-preset-angular@git://github.com/wtho/jest-preset-angular#test/tryout-babel-jest-angular

(make sure you followed the other instructions in the jest-preset-angular/babel README)

Please report any problems. Feel free to open issues on https://github.com/wtho/jpa-babel-bug

Thanks!

viceice · 2020-02-08T22:35:33Z

News: Angular 9 is out and they require @Injectable more aggressive.
https://angular.io/guide/migration-injectable

wtho · 2020-02-09T11:58:38Z

Finally!
I think that's no problem for the babel scenario (or the preset in general).

viceice · 2020-02-09T12:03:35Z

Yes, but fixes one of my bugs by design. 😊

ahnpnl · 2020-04-21T11:25:33Z

FYI: kulshekhar/ts-jest#1549 will be in alpha version of ts-jest (possibly today). You can test the alpha version and give some feedbacks for kulshekhar/ts-jest#1115

ahnpnl · 2020-10-27T10:18:21Z

I guess we can close this issue since we have to use Program

wtho · 2020-10-27T11:21:54Z

I think we can keep open just in case this gets interesting again. I'll make it a draft though.

wtho added 🚀 Feature Request new suggested feature Enhancement ✨ Question labels Oct 2, 2019

wtho added this to the 9.0.0 milestone Oct 2, 2019

wtho force-pushed the feat/babel-jest-angular branch from 4f34333 to d66ce40 Compare October 16, 2019 15:35

wtho force-pushed the feat/babel-jest-angular branch from d66ce40 to ceab978 Compare October 22, 2019 16:11

wtho marked this pull request as ready for review October 22, 2019 20:49

wtho force-pushed the feat/babel-jest-angular branch from ceab978 to d0f4ce0 Compare October 22, 2019 20:49

wtho force-pushed the feat/babel-jest-angular branch 7 times, most recently from 815c2d9 to 7be334a Compare October 25, 2019 20:40

wtho requested review from ahnpnl and thymikee October 25, 2019 20:49

ahnpnl reviewed Oct 27, 2019

View reviewed changes

package.json Show resolved Hide resolved

wtho force-pushed the feat/babel-jest-angular branch from 7be334a to 249aca1 Compare October 27, 2019 14:29

ahnpnl approved these changes Oct 28, 2019

View reviewed changes

BensonBen reviewed Dec 2, 2019

View reviewed changes

src/babel/load-html-transformer.ts Outdated Show resolved Hide resolved

wtho force-pushed the feat/babel-jest-angular branch from 249aca1 to e326cb6 Compare December 2, 2019 18:58

thymikee requested changes Dec 2, 2019

View reviewed changes

feat: make babel non-breaking

74fff02

thymikee reviewed Dec 3, 2019

View reviewed changes

wtho mentioned this pull request Feb 18, 2020

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

Closed

ahnpnl removed this from the next milestone Aug 23, 2020

wtho removed the Question label Oct 30, 2020

wtho marked this pull request as draft October 30, 2020 22:10

	* Add preset `babel` as transpilation alternative to `ts-jest` ([#317](https://github.com/thymikee/jest-preset-angular/pull/317)).
	* Expose a new preset `jest-preset-angular/build/babel` for transforming files with Babel instead of `ts-jest` ([#317](https://github.com/thymikee/jest-preset-angular/pull/317)).

		@@ -19,6 +22,19 @@ npm install -D jest jest-preset-angular @types/jest

		This will install `jest`, `@types/jest`, `ts-jest` as dependencies needed to run with Angular projects.

		Additionally you need `babel` packages if you want to use `babel`.

feat: alternative setup with babel/typescript #317

Are you sure you want to change the base?

feat: alternative setup with babel/typescript #317

Conversation

wtho commented Oct 2, 2019 • edited Loading

ahnpnl commented Oct 3, 2019

wtho commented Oct 3, 2019

wtho commented Oct 16, 2019

wtho commented Oct 22, 2019

ahnpnl commented Oct 23, 2019

wtho commented Oct 23, 2019 • edited Loading

wtho commented Oct 25, 2019

ahnpnl commented Oct 27, 2019

wtho commented Oct 27, 2019

ahnpnl commented Oct 28, 2019

wtho commented Oct 28, 2019

BensonBen commented Dec 2, 2019

thymikee commented Dec 2, 2019

wtho commented Dec 2, 2019

thymikee left a comment

Choose a reason for hiding this comment

wtho commented Dec 2, 2019

viceice commented Dec 3, 2019

wtho commented Dec 3, 2019

viceice commented Dec 3, 2019 • edited Loading

thymikee Dec 3, 2019

Choose a reason for hiding this comment

thymikee Dec 3, 2019

Choose a reason for hiding this comment

wtho Dec 3, 2019

Choose a reason for hiding this comment

thymikee Dec 3, 2019

Choose a reason for hiding this comment

thymikee Dec 3, 2019

Choose a reason for hiding this comment

thymikee Dec 3, 2019

Choose a reason for hiding this comment

wtho commented Dec 3, 2019

viceice commented Dec 4, 2019

viceice commented Dec 4, 2019

wtho commented Dec 20, 2019

viceice commented Feb 8, 2020 • edited Loading

wtho commented Feb 9, 2020

viceice commented Feb 9, 2020

ahnpnl commented Apr 21, 2020

ahnpnl commented Oct 27, 2020

wtho commented Oct 27, 2020

wtho commented Oct 2, 2019 •

edited

Loading

wtho commented Oct 23, 2019 •

edited

Loading

viceice commented Dec 3, 2019 •

edited

Loading

viceice commented Feb 8, 2020 •

edited

Loading