Ignore imports of types with typescript

[lgandecki]

Currently importing the types from a separate file creates a dependency, that will cause jest to run substantially more tests than it should be.

To show a specific case I created a semi-generated graphql server : where I did outside-in TDD development (and then replicated in two other types that are basically the same). This way I have ~250 tests that are completely decoupled. Besides the first-layer tests that hit the graphql directly, all the remaining tests are concerned with one class/file, and should preferably only run in a watch mode when that particular corresponding file changes. Similarly, all files (besides the graphql layer), should cause only a single test to run.

With this change applied and jest linked, when changing src/articles/articlesRepository.ts:

Without it (same file changed):

My solution/proposition is to use babel to strip the imports of the types, which will decouple files that are depending on types ("interfaces"), but not implementations.

This was an idea that came to me after an enlightening (for me) discussion with @searls @marcpeabody and @samhatoum around the topic of Dependency Inversion/using interfaces that will be implemented by single classes, while writing documentation about using TestDouble with TypeScript.

I acknowledge that there is a performance hit on the start, but it happens only the first time jest is ran in the given project, after that the hastemap cache is doing an amazing job of keeping everything very snappy.

I also acknowledge, that this might be a rare usecase, but maybe with a change like this more people would be willing to look at it? I think the advantages for a Test Driven workflow are significant.

If there is no chance of merging it like this, maybe some way to opt-in for this behavior? I wish I was able to just use the dependencyExtractor for something like this, but it only takes text, so it doesn't know if a given import is for a type, or for something concrete.

Another suggestion would be to allow dependencyExtractor to take a file path instead of code as a text.

Thank you guys!

Test plan

I added the tests, although I'm pretty sure there is a better place to add them, please advise. I treat this as a WIP/proposal open for discussion.

[SimenB]

Thanks for the PR!

I wish I was able to just use the dependencyExtractor for something like this, but it only takes text, so it doesn't know if a given import is for a type, or for something concrete.

Not sure why you can't do your current thing in a custom extractor?

// my-dependency-extractor.js
module.exports.extract = (content, defaultExtractor) => {
  const transformed = babel.transformSync(content, {
    cwd: __dirname,
    plugins: ['@babel/plugin-transform-typescript']
  });

  return defaultExtractor(transformed ? transformed.code : content);
};

(We have a PR giving you the filepath as well, see #7362)

/cc @rubennorte who's probably had thoughts about this

[lgandecki]

thanks for a prompt reply @SimenB ! Yeah, if I had access to the filePath, that would be exactly my solution. But do I ? Where would it come from? As far as I saw, and also your snippet shows, I only get content and defaultExtractor.. Please prove me wrong though, that would be fantastic :)

[SimenB]

See my update, it's coming via #7362, which will land before Jest 24 (we just haven't landed on the API)

[lgandecki]

oh, I see. Great. :) I will be watching that one as well.

[jeysal]

It's amazing how easy it is to implement something like this (regardless of whether this gets merged). 2 years ago, Babel didn't even have typescript support.

There is a way to make this a bit faster:
Babel finds out which imports are types by checking whether they are only referred to in AST positions where types go.
The dependency extractor could use Babel to traverse the AST and do the same, but do not modify anything and do not generate output code.
It does require parsing though, so it's probably still orders of magnitude slower than the regex matching that the default dependency extractor does.

[SimenB]

The current approach is out of the question - we're not going to parse any files in jest-haste-map - TS or not. However, I think #7362 will solve this issue correctly (and maybe we could link to some modules from the docs that does this (stripping out type imports, seems useful since we don't typecheck anyways))

[lgandecki]

Right, @jeysal ! Amazing :) And it only gets better..

I was afraid of the performance issues, but to be honest they were minimal, for the few repos that I tried. And, because of the caching and it's one-time-cost, it doesn't really matter that much, at least for me, but I suspect it would be true for a lot of other people.

Interesting thought with the fact that there might be a way to actually remove the type imports just looking at the string of the code itself. I somehow assumed it does look at the dependencies! I will try to implement it the way you are proposing, thanks for the pointer! I tried to run @babel/plugin-transform-typescript on the string itself, but it doesn't seem to work without having the path.. so I assumed the file is a must :)
I might create a very small babel plugin then, that only removes the import types..

edited:
btw, my performance jump, as seeing in the screenshots was from 6s to 0.06s, so in this case even a 50% one-time startup delay would really be unimportant. :)

[rubennorte]

As @SimenB said, we can't parse files by default in Jest, as that'd considerably degrade startup performance (big repos are highly affected by this). I'm going to merge #7362 soon and it'll be available in 24 so you can implement it if you need it.

That said, I don't see how your changes strip type imports. I don't know much about TS but, as opposed to Flow where you have type imports (import type ... from "foo") and you can determine what are type imports by just analyzing the file, I don't see a way to do it with TS (rather than using import types).

[jeysal]

That said, I don't see how your changes strip type imports. I don't know much about TS but, as opposed to Flow where you have type imports (import type ... from "foo") and you can determine what are type imports by just analyzing the file, I don't see a way to do it with TS (rather than using import types).

@rubennorte FYI see my comment above, the way @babel/plugin-transform-typescript does it is look at where those imports are used in the file, and if its only in places where types go and not regular identifiers etc., it strips the imports out. Works pretty well, although I still perceive Flow's import type as a bit cleaner.

[rubennorte]

@jeysal oh, I see. I guess it ignores possible side-effects in the imported module, but that should be fine in most cases. Thanks for the clarifications.

I just merged #7362 so you'll be able to use it in 24 to implement this.

[lgandecki]

Hey @rubennorte .
If you only import types from a file, there are no side-effects, because the imports will be stripped before runtime, that's the whole point. :-)

[rubennorte]

@lgandecki if the imported module caused a side-effect in the global scope (either directly or through a transitive dependency) and the code relied on that, stripping the import would cause a runtime error. E.g.:

foo.js

global.fooVar = 'defined!';
export type Foo = string;

bar.js

import {Foo} from './foo';

const value: Foo = 'hello';
console.log(global.fooVar);

[lgandecki]

@rubennorte right. but the imports are indeed removed before the code runs, so yes, you do get a runtime error, it's not like you have a choice.
This is what the compiled version of your bar.ts looks like:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const value = "hello";
// @ts-ignore
console.log(global.fooVar);

You are saying that it should be fine in most cases - it's just you can't rely on the fact that you import types, if you want side-effects, ever.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.