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

Skott does not read typescript path aliases if entrypoint is not a TS file #84

Closed ACHP closed 1 year ago

ACHP commented 1 year ago

Context

I tried s skott on a React-Native project where almost all files are using Typescript except for the entry file. And I noticed that all my path aliases were considered as "third party module". Even with the --tsconfig="./tsconfig.json" argument.

How to reproduce

That's because if the entry point is not a TS file, the TS configuration is not loaded See here:

https://github.com/antoine-coulon/skott/blame/d18c1f800276752431d0177597d1fa4f86db5938/packages/skott/src/skott.ts#L494-L502

Suggestion

If the user use tsconfig as argument for the cli, skott should consider that the project is using typescript

antoine-coulon commented 1 year ago

Hello @ACHP, thanks for opening that very well detailed issue!

It's actually an interesting use case, but I'm curious to know why would a JavaScript file be the entrypoint of a TypeScript project in the first place?

Using the tsconfig option as way to settle that the current target is actually TypeScript would be I think a bit cumbersome because internally the tsconfig option is always tsconfig.json by default so we wouldn't easily know if tsconfig.json comes from user input or not.

Maybe an even more straightforward solution would be to try to build path aliases all the time as skott might support #78 Node.js path aliases (https://nodejs.org/api/packages.html#subpath-imports) as well. I'm not really a fan of doing this type of thing but actually with all the mixes that can happen in a JS/TS project with allowJs and all the different alias configs that can be setup I think that determining alias strategy based on the entrypoint would be a mistake.

ACHP commented 1 year ago

why would a JavaScript file be the entrypoint of a TypeScript project in the first place?

Because we can … 😆 More seriously, I don't think that this is something a user would like to do on purpose, but it can happen with allowJS For my personal use case, the reasons are:

I don't think this will happens really often, but when it will, user will have to debug skott source code only to find out they need a typescript entry point

Using the tsconfig option as way to settle that the current target is actually TypeScript would be I think a bit cumbersome because internally the tsconfig option is always tsconfig.json by default so we wouldn't easily know if tsconfig.json comes from user input or not.

Yes, you're right. I tried to implement a fix yesterday after opening the issue, and as you mention tsconfig is always declared so we can't use this approach.

Maybe an even more straightforward solution would be to try to build path aliases all the time as skott might support https://github.com/antoine-coulon/skott/issues/78 Node.js path aliases

I think there are 2 distinct issue here:

I think we first need to correctly load config that skott support ( for now Typescript only ), then, think of a strategy to "merge" or select one aliases config over another.

Something like

private async buildFromEntrypoint(entrypoint: string): Promise<void> {

    if (isTypeScriptProject()) {
      this.#workspaceConfiguration.typescript = await buildTSConfig(/*...*/);
    }
    // later
    if (isBabelProject()) {
      this.#workspaceConfiguration.babel = await buildBabelConfig(/*...*/);
    }
    // later
    if (hasJSConfig()) {
      this.#workspaceConfiguration.jsonConfig = await buildJSConfig(/*...*/);
    }
    this.#workspaceConfiguration.pathAliases = readPathAliasesFrom([this.#workspaceConfiguration.typescript, this.#workspaceConfiguration.babel, this.#workspaceConfiguration.jsonConfig]);
  /*...*/
}

This is, IMO, one possible way to do this, if you plan to add multiple pathAliases resolution strategy.

For now if you keep typescript only you can just keep your actual code and replace isTypescriptModule(entryPoint) by isTypescriptProject().

isTypescriptProject() can simply try to resolve the config.tsConfig variable, if the tsconfig file exist, load the config, otherwise skip it


function isTypeScriptProject(){
  // naïve "generic" approach
  return fs.existsSync(config.tsConfig);
}

private async buildFromEntrypoint(entrypoint: string): Promise<void> {

    if (isTypeScriptProject()) {
      this.#workspaceConfiguration.typescript = await buildTSConfig(/*...*/);
    }

  /*...*/
}
antoine-coulon commented 1 year ago

Because we can … 😆 More seriously, I don't think that this is something a user would like to do on purpose, but it can happen with allowJS [...]

Yeah you can do many things, even executing SQL inside CSS, that ecosystem is burning my brain sometimes 😆

By the way I was not judging in any kind of way there, just curiously wondering what was the setup because I tried to anticipate as much scenarios as possible but this missed this one.

Anyway I think this type of use case and even the possibility of turning on allowJs as previously mentioned is enough to confirm that whatever decision skott takes, it should not be based upon the entrypoint.

I think there are 2 distinct issue here:

How to check that a project is using typescript (and maybe load typescript path aliases) And what kind of path aliases can be specified ( jsconfig, webpack aliases, babel aliases, package.json aliases, typescript aliases etc ...).

Yeah absolutely! We'll definitely need at some point to have this type of strategy to resolve path alias configurations from different sources (tsconfig.json, package.json, vite.config.js...).

For now if you keep typescript only you can just keep your actual code and replace isTypescriptModule(entryPoint) by isTypescriptProject() [...]

Yes you're right, that's pretty much what I had in mind feature-wise :)

Regarding the implementation what I was saying try to build all the time path aliases was mostly equivalent to isTypeScriptProject(), the difference being that building path aliases would silently fail if the current project is not a TypeScript one hence not building path aliases (but providing the same result).

Also, we must be careful semantically because it's hard to settle whether a given project is TypeScript or not based only on tsconfig.json because one could provide TypeScript compiler options via CLI without any tsconfig.json file and it would still be a TypeScript project. Consequently, what I'd suggest for now (taking into account that we only have one path alias strategy to deal with) is to try to build TypeScript path aliases in all scenarios, if there is no tsconfig.json the computation will just silently fail. It's like fire and forget. In that case, we might just need to update the logger error message https://github.com/antoine-coulon/skott/blob/d18c1f800276752431d0177597d1fa4f86db5938/packages/skott/src/modules/walkers/ecmascript/typescript/path-alias.ts#L172 to avoid misleading users not using TypeScript or not expecting some random tsconfig.json to be searched.

For the future dealing with multiple strategies, we might just need to remove --tsconfig in favor of a flag that maps to whatever supported file that can declare path aliases (tsconfig, package.json, vite.config.js...), because currently tsconfig is only used to look for path aliases, nothing else. From that unique source of truth we'll then be able to construct alias references and keep most of the current logic the same.

ACHP commented 1 year ago

Totally agree with all of this :) Just some notes though:

antoine-coulon commented 1 year ago

Yeah you're right both fit well for now, so let's go on the solution you suggested, I think it's pretty good for the first iteration!

Would be willing to land a PR and contributing on this subject and/or the other issue you opened #83? It's totally up to you, on my side I'm already grateful for the discussion and the opening of the issues, so I could do it myself as well but mainly working on #72 so it could take a bit longer for me to land it :)

ACHP commented 1 year ago

Yes, I'll do that ;) I'll will probably start with a naïve approach first so we can talk about it in the PR comments (And I'm I'm not familiar with EffectTS for now).

antoine-coulon commented 1 year ago

Glad to hear that, sounds good to me! You can go full TypeScript if you're more comfortable with it, I'm mainly using Effect for dependency injection on skott for now so nothing really fancy. If you ever want to know more though, I authored an introduction about it https://github.com/antoine-coulon/effect-introduction :)

antoine-coulon commented 1 year ago

Should be fixed by #89 released in 0.28.3. Waiting for @ACHP to check that it works his setup before closing the issue.

ACHP commented 1 year ago

Works great ! 🚀 🚀 🚀

Now I got some mess to clean

image
antoine-coulon commented 1 year ago

@ACHP nice, glad to see that you made it work! Now I need to speed up the revamp #72 because that visualization is getting ugly and messy for big graphs... To improve the graph visualization I would suggest you to filter a little bit the parts of your project so that you reduce the number of nodes displayed (either via ignorePattern or cwd to select a sub directory). Hopefully the next webapp version will erase all these drawbacks and make the visualization smooth, even for big projects.

antoine-coulon commented 1 year ago

Closing this at it was solved in #89

Thanks again for the nice work @ACHP, appreciate the contributions :)