webpro-nl/knip

🐛 Jest plugin should allow ts-node dependecy when using Typescript config

bigpopakap opened this issue · 6 comments

bigpopakap commented

Prerequisites

Reproduction url

https://stackblitz.com/edit/github-rwe1kn?file=README.md

Reproduction access

  • I've made sure the reproduction is publicly accessible

Description of the issue

How the repro was set up

  1. npm install -D jest, install Jest
  2. npm init jest@latest, and selecting Typescript to use .ts for the config (creating jest.config.ts)
  3. Added test script in package.json to run Jest tests
  4. Added one example test example.test.js, so npm run test works
  5. Then installed ts-node (keep reading)

The issue with Jest

Jest requires ts-node to run tests when using the Typescript config file jest.config.ts.

If you don't, you'll get the following error in the console

~/projects/rlmibmmqya.github
❯ npm run test

> @knip/templates-basic@1.0.0 test
> jest

Error: Jest: Failed to parse the TypeScript config file /home/projects/rlmibmmqya.github/jest.config.ts
  Error: Jest: 'ts-node' is required for the TypeScript configuration files. Make sure it is installed

The issue with Knip

Now if you install ts-node with npm install -D ts-node, tests will run and pass

~/projects/rlmibmmqya.github 7s
❯ npm run test

> @knip/templates-basic@1.0.0 test
> jest

 PASS  ./example.test.js
  ✓ should run this test (1 ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        0.219 s

However, running Knip will now flag ts-node as an unused dependency

~/projects/rlmibmmqya.github 2s
❯ npm run knip

> @knip/templates-basic@1.0.0 knip
> knip

Unused devDependencies (1)
ts-node  package.json

Proposed solution

Knip's Jest plugin should detect what kind of Jest config is used. If it's Typescript, it should allowlist the ts-node dependency.

bigpopakap commented

Hmm... maybe this should actually be tagged as a feature request? I'm not sure 🤷 Of course, please feel free to relabel as needed

webpro commented

Knip's Jest plugin should detect what kind of Jest config is used. If it's Typescript, it should allowlist the ts-node dependency.

If Knip would do that, then ts-jest users might be unpleasantly surprised, no?

Could there be another way to detect ts-node is required maybe?

Alternatives:

  • Use ts-jest
  • Add ts-node to ignoreDependencies: []
bigpopakap commented

Yeah, I currently just put ts-node in ignoreDependencies myself. I thought maybe the plugin should do that.

I completely forgot about ts-jest - I always found it annoying compared to jest+ts-node, so I haven't used ts-jest in years. Anyway, that's just personal preference. I agree it's not good to assume ts-node is the only way.

But given that using jest.cofig.ts requires some sort of Typescript dependency, it does feel worth including in a plugin. Some questions and discussion topics:

  • are ts-node/ts-jest the only such dependencies? If so, more reason to enforce it because it's simple. If there are many different ways a user could use a Typescript config, then probably not worth enforcing
  • Is there any room for having some sort of OR rule? If you use Typescript for your config, and have ts-jest, then ts-node is not allowlisted. But if you don't have ts-jest, then ,ts-node is allowlisted. I can see this logic getting complex, and it does arbitrarily prioritize one dependency over the other. Thoughts?
  • Is ts-jest allowlisted in the plugin right now? If so, that's making the opposite assumption. Eg. For me, I don't want ts-jest, but maybe the plugin in allowlisting it. (I'll check when I get to my computer)
webpro commented

Typically, cases where this type of magic happens I'm inclined to advice and add it to ignoreDependencies. One could argue it's an (optional) peer dependency.

What if if you're running it with something like tsx jest or node --experimental-strip-types jest or with Bun? Then some of the logic might fall apart and/or become more complex.

Another thing to consider is that Knip also looks for unlisted dependencies, which doesn't go great with the idea of "allowlist".

bigpopakap commented

Yeah, this all makes sense. I guess this is one more case where it makes the most sense for a user to add to ignoreDependencies.

So I agree. This can be closed as "won't do", and it just stays in userland ignoreDependencies

More thoughts:

It's almost like an optional peer dependency, but it is semi-required. It's like a peer dependency group, where you have to use some method to support Typescript in Jest. But as you mention, the choice of method is pretty open-ended.

Re: "allowlist" what's a better term for it? I was struggling to describe that in a succinct way, so that was the best I could come up with

webpro commented

Yeah, this all makes sense. I guess this is one more case where it makes the most sense for a user to add to ignoreDependencies.

So I agree. This can be closed as "won't do", and it just stays in userland ignoreDependencies

Great, let's go with that.

More thoughts:

It's almost like an optional peer dependency, but it is semi-required. It's like a peer dependency group, where you have to use some method to support Typescript in Jest. But as you mention, the choice of method is pretty open-ended.

Package management is not trivial :)

Re: "allowlist" what's a better term for it? I was struggling to describe that in a succinct way, so that was the best I could come up with

It's a good term imho, it's just that Knip does not "allow" things yet is more binary in that something is referenced or not. And then listed or not.