antoine-coulon / skott

All-in-one devtool to automatically analyze, search and visualize project modules and dependencies from JavaScript, TypeScript (JSX/TSX) and Node.js (ES6, CommonJS)
MIT License
669 stars 25 forks source link

Resolving "import type" statements #155

Closed kevin-st closed 6 months ago

kevin-st commented 6 months ago

Summary

In a project I'm working on, I currently have quite a lot of circular dependencies and one of them is causing the application to crash. I noticed that a lot of these statements are importing a value, while the thing being imported is only being used as a type.

I found out that one could also write import statements as import type { something } from "module", which would result in the import statement being removed after compilation. I guess it's now up to us to optimize our import statements.

This left me wondering: does Skott do the right thing with these import statements? I guess the answer is no.

Details

I hereby refer again to the skott-example repository (on the branch import-type-statement-demo): https://github.com/kevin-st/skott-example/tree/import-type-statement-demo

In this demo I added a person folder in which a few scripts can be found:

After running yarn dev, we can see that addPerson and interface-demo don't contain an import statement in the bundled code, while createPerson does. But if we take a look at the output of Skott, then we see that it draws an arrow between the modules:

afbeelding

While I was testing I also created a cyclic dependency on purpose and skott recognized this, but I guess this should be seen as a false positive since the import statements might be removed after compilation depending on the statement.

I guess that a case like import type Person from ./Person would be possible to catch, but that a case like import { IPerson } from "./Person" is a bit more difficult, since you don't know whether the thing being imported is a type/interface or an actual value, unless I'm wrong.

So my question is: is Skott behaving the way it should or should we catch (at least) these import type statements (if possible)?

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question Answer
Would you consider contributing a PR?
antoine-coulon commented 6 months ago

Hello @kevin-st

This left me wondering: does Skott do the right thing with these import statements? I guess the answer is no.

From what you described, it seems that skott is doing the right thing which is using the current TypeScript source code to draw dependencies, and not trying to guess what would happen after transpiling/bundling files.

While I was testing I also created a cyclic dependency on purpose and skott recognized this, but I guess this should be seen as a false positive since the import statements might be removed after compilation depending on the statement.

As it is stated in the documentation, tracking cyclic dependencies is not only useful to avoid runtime issues but also helps for design purposes and to reveal code smells. If you want to only track dependencies that will be there at runtime, you can use skott --no-trackTypeOnlyDependencies which will have for effect to completely discard import type { A } and import { type A } dependencies from the graph.

So my question is: is Skott behaving the way it should or should we catch (at least) these import type statements (if possible)?

skott by default will track the most information possible including type-level information, and I personally find that to be useful, not for runtime purposes, but for design purposes where cyclic dependencies might reveal design smells (this is why this is enabled by default). Sometimes TypeScript might go crazy on type resolution with cyclic dependencies and the compiler might produce errors, so better to avoid them when possible.

Nonetheless it's up to you and you are free to consider them important or not :) I would suggest you to have two different CLI config if the cycles related to types are too noisy, so that you can focus on the ones that will likely be causing runtime issues.

kevin-st commented 6 months ago

Hi @antoine-coulon

I guess it can be interesting to track everything for a project which you're starting up, but since this project was already being worked on for quite some time and given the fact that it's also quite large, tracking everything lead to an enormous amount of circular dependencies, which made it quite hard to find the culprit.

That being said, using the --no-trackTypeOnlyDependencies flag helped to focus on those relations which only exist at runtime and I've managed to bring it down from more than 30 to only 3 circular dependencies, fixing the one relationship which was causing the application to crash. Those last 3 are kind of difficult and maybe even impossible to fix for us, since we are working on an application which is based on a database which is out of our control.

For example: in the database we have a table called articles which links to anglzone and if a record in anglzone has divdir set to "A", then we link back to articles.

We are very tightly coupled to that database and we follow those relations in our TypeScript code, so I guess some of these dependencies just can't be fixed as far as I can see it atm.

Anyhow, thanks for the help!

antoine-coulon commented 6 months ago

@kevin-st glad to know you could isolate what mattered for you. Don't hesitate to open other discussions if you see any improvement that could make your life easier when dealing with dependencies!

I'll also add more documentation around this topic.