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

feat: alternative setup with babel/typescript #317

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wtho
Copy link
Collaborator

@wtho wtho commented Oct 2, 2019

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

@wtho wtho added this to the 9.0.0 milestone Oct 2, 2019
@ahnpnl
Copy link
Collaborator

ahnpnl commented Oct 3, 2019

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

@wtho
Copy link
Collaborator Author

wtho commented Oct 3, 2019

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

@wtho
Copy link
Collaborator Author

wtho commented Oct 16, 2019

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
Copy link
Collaborator Author

wtho commented Oct 22, 2019

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!

@wtho wtho marked this pull request as ready for review October 22, 2019 20:49
@ahnpnl
Copy link
Collaborator

ahnpnl commented Oct 23, 2019

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
Copy link
Collaborator Author

wtho commented Oct 23, 2019

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 wtho force-pushed the feat/babel-jest-angular branch 7 times, most recently from 815c2d9 to 7be334a Compare October 25, 2019 20:40
@wtho
Copy link
Collaborator Author

wtho commented Oct 25, 2019

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.

@wtho wtho requested review from ahnpnl and thymikee October 25, 2019 20:49
@ahnpnl
Copy link
Collaborator

ahnpnl commented Oct 27, 2019

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 Show resolved Hide resolved
@wtho
Copy link
Collaborator Author

wtho commented Oct 27, 2019

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
Copy link
Collaborator

ahnpnl commented Oct 28, 2019

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
Copy link
Collaborator Author

wtho commented Oct 28, 2019

👍 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
Copy link

@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
Copy link
Owner

thymikee commented Dec 2, 2019

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

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
Copy link
Collaborator Author

wtho commented Dec 2, 2019

@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.

Copy link
Owner

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

wtho commented Dec 2, 2019

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
Copy link

viceice commented Dec 3, 2019

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

ts-jest
image

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

image

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

diagnostics: false,
isolatedModules: true,

@wtho
Copy link
Collaborator Author

wtho commented Dec 3, 2019

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
Copy link

viceice commented Dec 3, 2019

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();
    });
});

* 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).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not relevant now

### v8.1.0

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove, no need to mention it here

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

@@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

Suggested change
### babel
### jest-preset-angular/babel

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.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

wtho commented Dec 3, 2019

@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
Copy link

viceice commented Dec 4, 2019

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

@viceice
Copy link

viceice commented Dec 4, 2019

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

second run
image

@wtho
Copy link
Collaborator Author

wtho commented Dec 20, 2019

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
Copy link

viceice commented Feb 8, 2020

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

@wtho
Copy link
Collaborator Author

wtho commented Feb 9, 2020

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

@viceice
Copy link

viceice commented Feb 9, 2020

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

@ahnpnl
Copy link
Collaborator

ahnpnl commented Apr 21, 2020

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 ahnpnl removed this from the next milestone Aug 23, 2020
@ahnpnl
Copy link
Collaborator

ahnpnl commented Oct 27, 2020

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

@wtho
Copy link
Collaborator Author

wtho commented Oct 27, 2020

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

@wtho wtho removed the Question label Oct 30, 2020
@wtho wtho marked this pull request as draft October 30, 2020 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alternate setup with @babel/preset-typescript without ts-jest
5 participants