More safely resolve 'jest-config'.

[iclanton]

ts-jest seems to assume that consumers are using npm or yarn, which both flatten the node_modules folder and allow packages to import dependencies that are not declared in their package.json. The flattened node_modules folder assumption doesn't hold true for other package managers like pnpm and yarn's plug-and-play feature.

jest-config is imported on this line, but jest-config isn't declared explicitly in ts-jest's package.json. This wasn't a problem in version 22. This PR changes the resolution algorithm for jest-config to find the specific version that jest itself is using.

We also considered adding a peerDependency on jest-config, but that requires that the consuming package needs to take a direct dependency on jest-config, and coordinate its version of jest-config to be the same as the version used by jest.

We aren't using the ts-jest preset, so it would actually be more efficient for our toolchain to reference an entrypoint in ts-jest that only exports the transform and nothing else in the module graph. If you're open to that change, I'll submit a PR that creates an entrypoint that only exports the transform.

[iclanton]

@pgonzal

[octogonz]

Thanks for fixing this @iclanton!

[mikew]

Just curious, why isn't jest-config in this package's dependencies? It's one of the first things this package tries to import.

[iclanton]

jest-config is transitively in this package's peer dependencies (jest -> jest-cli -> jest-config). If jest-config was listed as a peer dependency of ts-jest, then consumers of ts-jest would have to explicitly list jest-config as a dependency, which is unnecessary (because it's already a dependency of jest, and jest is already a peer dependency), and requires that the consumer keep the version of jest-config aligned with the version of jest, which is likely to lead to mistakes.

Making jest-config a dependency of this package is likely to cause the wrong version of jest-config to be installed for the version of jest that the consumer of this package wants.

[coveralls]

Pull Request Test Coverage Report for Build 2173

• 25 of 27 (92.59%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 90.943%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/config/jest-config-resolver.ts	21	23	91.3%

Totals
Change from base Build 2160:	0.02%
Covered Lines:	1111
Relevant Lines:	1163

---

💛 - Coveralls

[huafu]

@iclanton thanks for the PR. I'll try to spend some time on it this Sun/Mon. Tho next time, don't bypass the commit hooks (it should have fail committing since your commit messages are not following the conventional commit format we are using) ;-)

[iclanton]

I couldn't figure out what the hook's expected commit format was, so I assumed the hook wasn't working correctly. Here's the output:

input:
'Fix issue where symlinked package directories weren't resolved.'

✖ message may not be empty [subject-empty]
✖ type may not be empty [type-empty]
✖ found 2 problems, 0 warnings

I can edit the commit messages and force push my branch. What is the format supposed to be?

[huafu]

@iclanton that would be awesome.
here are specs https://www.conventionalcommits.org/en/v1.0.0-beta.2/#summary
and for the types: https://github.com/angular/angular/blob/22b96b9/CONTRIBUTING.md#type

Also if you could rename any var/exported symbol which is jestConfig into something like jestConfigPkg that would be nice, because else it can be miss-read as the jest configuration instead of the lib.

[iclanton]

Done :)

[ahnpnl]

LGTM, but do you have some tests for this ?

[huafu]

There are many spots where you could just let your variable types be inferred from the return type of function calls on initialisation.

Other than that and the few spots where I left an inline review, LGTM.

[huafu]

We use to start a type with an uppercase, and prefix with a T when it's a package export, so here I guess it should be named TJestConfigPkg

Anyway, here the getJestConfigPkg should be defined in a way so that the returned type is inferred here (I yet did not look at it in the jest-config-resolver-file). Long story short, you shouldn't have to import * as jestConfigType and here should only be const jestConfigPkg = getJestConfigPkg(logger). Type will be inferred from function.

[huafu]

Ok, just saw now, the type IS returned from getJestConfig, so simply remove the import and replace that line with what I wrote you.

[iclanton]

The type isn't implied because the getJestConfig is generic. I'll change it to not be generic and use the TJestConfigPkg name you suggested.

[iclanton]

Actually I remember why I did this the way I did it - giving getJestConfigPkg a non-generic return type and returning the real type (typeof TJestConfigPkg), the jest-config-resolver.d.ts declaration file takes a dependency on jest-config, which means that ts-jest can't be referenced by other TypeScript projects that don't have a flattened node_modules. I'm going to put it back the way I had it before to avoid this issue if that's okay with you.

[huafu]

Oh yeah, good point! Tho if the type you are using is just defined for this file, move it out of types.ts and into this file.

[octogonz]

There are many spots where you could just let your variable types be inferred from the return type of function calls on initialisation.

@huafu It is much more valuable for code to be easy to read than easy to write: When other developers have to make extra effort to understand a file, it's more difficult for them to do a thorough code review, or to figure out how to contribute fixes. VS Code has a really nice IntelliSense feature that shows the inferred types, but often the person reading the code may be using a simple viewer such as GitHub or a visual diff/merge tool.

So, I'd suggest not to rely toooo heavily on type inference, as it hides information that is often not obvious when reading someone else's code (even though it was probably completely obvious to the original author.)

[huafu]

I agree, but for what I said the function name must be chosen so that it's clear enough to infer from the name. If not, one must change everything, as for readability it is also important to have the code written with same style everywhere.

[iclanton]

Do you think this can be merged in and released soon? We have some work that's blocked behind this.

[octogonz]

@huafu after a PR is already approved, what else is required to get the PR merged?