antoine-coulon / krush

🏍 The set of plugins that will make you love Rush even more
4 stars 0 forks source link

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. #1

Open antoine-coulon opened 1 year ago

antoine-coulon commented 1 year ago

Moving the issue created by @kaiyoma here:

For starters, this tool hangs indefinitely when run in our repo. We discovered that for some reason, we have to delete a .docusaurus directory from one of our packages to get this tool to finish at all. After doing that, the tool crashes when run:

$ rush graph

Rush Multi-Project Build Tool 5.93.2 - https://rushjs.io/
Node.js version is 16.19.1 (LTS)

Trying to acquire lock for pnpm-7.29.3
Acquired lock for pnpm-7.29.3
Found pnpm version 7.29.3 in C:\Users\kgetz\.rush\node-v16.19.1\pnpm-7.29.3

Symlinking "C:\Users\kgetz\Work\event-viewer\common\temp\pnpm-local"
  --> "C:\Users\kgetz\.rush\node-v16.19.1\pnpm-7.29.3"
Acquiring lock for "common\autoinstallers\rush-plugins" folder...
Autoinstaller folder is already up to date

Starting "rush graph"

✓ Rush configuration found
TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
    at new NodeError (node:internal/errors:387:5)
    at validateString (node:internal/validators:162:11)
    at Object.join (node:path:429:7)
    at resolveAliasToRelativePath (file:///C:/Users/kgetz/Work/event-viewer/common/autoinstallers/rush-plugins/node_modules/.pnpm/skott@0.17.3/node_modules/skott/dist/src/modules/walkers/ecmascript/typescript/path-alias.js:37:36)
    at resolvePathAlias (file:///C:/Users/kgetz/Work/event-viewer/common/autoinstallers/rush-plugins/node_modules/.pnpm/skott@0.17.3/node_modules/skott/dist/src/modules/walkers/ecmascript/typescript/path-alias.js:51:16)
    at EcmaScriptDependencyResolver.resolve (file:///C:/Users/kgetz/Work/event-viewer/common/autoinstallers/rush-plugins/node_modules/.pnpm/skott@0.17.3/node_modules/skott/dist/src/modules/resolvers/ecmascript/resolver.js:81:40)
    at Skott.collectModuleDeclarations (file:///C:/Users/kgetz/Work/event-viewer/common/autoinstallers/rush-plugins/node_modules/.pnpm/skott@0.17.3/node_modules/skott/dist/src/skott.js:182:47)
    at async Skott.buildFromRootDirectory (file:///C:/Users/kgetz/Work/event-viewer/common/autoinstallers/rush-plugins/node_modules/.pnpm/skott@0.17.3/node_modules/skott/dist/src/skott.js:262:13)
    at async Skott.initialize (file:///C:/Users/kgetz/Work/event-viewer/common/autoinstallers/rush-plugins/node_modules/.pnpm/skott@0.17.3/node_modules/skott/dist/src/skott.js:270:13)
    at async skott (file:///C:/Users/kgetz/Work/event-viewer/common/autoinstallers/rush-plugins/node_modules/.pnpm/skott@0.17.3/node_modules/skott/dist/index.js:8:27) {
  code: 'ERR_INVALID_ARG_TYPE'
}
antoine-coulon commented 1 year ago

This issue probably happens because the plugin is not updated with the latest skott version yet. The latest plugin version is using skott@0.17.3 but we're already at the skott@0.20.2. I'll do the bump and release and it should fix the issue

antoine-coulon commented 1 year ago

Just released the 0.2.0 version of the plugin that uses the latest version of skott. @kaiyoma can you please update the plugin version on your side and let me know if that fixes your issue? Thanks

kaiyoma commented 1 year ago

Same problems. Runs forever and never finishes unless I delete the Docusaurus folder, and even when I do, it errors out in the same way.

antoine-coulon commented 1 year ago

Alright, thanks for the information and btw thanks for reporting the issue @kaiyoma.

Looking at the stack trace it seems to be related to TypeScript path aliases, can you please paste me some of the tsconfig.json you're using?

About Docusaurus I'm not really familiar with it, the current limitations with skott is that it does parse all the JS/TS files without taking into account tsconfig include property (it only excludes arbitrary folders atm, I'll open an issue on skott's side to take that into account, I guess .docusaurus is git ignored on your side), so in the case where there are a lot of heavy generated/minified files, it could be really slow hence making the cli hang for a long time.

Could you also please try to run npx skott (preferably with the latest version) instead of rush graph at the root of the monorepo and tell me if you encounter the same problem?

kaiyoma commented 1 year ago

Our path aliases in our root tsconfig are pretty normal I think (slightly redacted):

      "actions/*": ["src/actions/*"],
      "components/*": ["src/components/*"],
      "help-center": ["lib/help-center/dist"],
      "help-center/*": ["lib/help-center/dist/*"],
      "hooks": ["src/hooks"],
      "hooks/*": ["src/hooks/*"],

Yes, this .docusaurus folder is git-ignored because it's part of the output of the build. We use include very heavily in all our tsconfigs because you can encounter some really bad performance problems if you don't. I learned this lesson the hard way a year or two ago and now we always specify include to prevent TS-based tools from crawling parts of the repo they shouldn't be looking in.

I see the same error and stack trace if I run npx skott directly.

antoine-coulon commented 1 year ago

Our path aliases in our root tsconfig are pretty normal I think (slightly redacted):

Yes, they are. I'm not seeking for a misconfiguration on your side but rather a specific pattern that would not be supported yet. From what I tested in a little repro using that config, I wasn't able to reproduce that error. Might be somewhere else.

Yes, this .docusaurus folder is git-ignored because it's part of the output of the build.

Cool, there is an on-going issue that will discard entries listed .gitignore files, so it won't be problematic anymore.

We use include very heavily in all our tsconfigs...

"include" is unfortunately not supported in skott as the initial goal was to be able to "crawl" everything that could be parsed (js/ts) by being config-less by default and emit graphs including potentially independent set of files or even independent set of projects (in the context of a monorepo). Consequently as of now, skott might traverse files that you don't expect to be analyzed as it will ignore the include property from the tsconfig.json. Moreover having a way of dealing consistently with include properties in a monorepo context would not be trivial because there might be a lot of differences between the way tsconfig.json are named/located for each project and making that configurable from a CLI would be just awful. So for now I'd prefer to black-list (using .gitignore) instead of white-listing entries to include. There is also an issue that aims to provide that black-listing in addition to the .gitignore that will be found through all the file tree.

I see the same error and stack trace if I run npx skott directly.

Well, that's annoying. I'm kind of frustrated of not being able to help fixing that issue right away tbh. Would you mind sharing the following information please:

Thanks

kaiyoma commented 1 year ago
$ npx skott -v
skott, 0.20.2

 Skott exited with code 0
$ rm -rf apps/cloudvision-docs/.docusaurus/ && npx skott

 Running Skott from current directory: event-viewer
⠹ Initializing Skott
 Skott exited with code 1
node:internal/errors:478
    ErrorCaptureStackTrace(err);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
    at new NodeError (node:internal/errors:387:5)
    at validateString (node:internal/validators:162:11)
    at Object.join (node:path:429:7)
    at resolveAliasToRelativePath (file:///C:/Users/kgetz/AppData/Local/npm-cache/_npx/c24e6b3ef94a6d87/node_modules/skott/dist/src/modules/walkers/ecmascript/typescript/path-alias.js:42:36)
    at resolvePathAlias (file:///C:/Users/kgetz/AppData/Local/npm-cache/_npx/c24e6b3ef94a6d87/node_modules/skott/dist/src/modules/walkers/ecmascript/typescript/path-alias.js:56:16)
    at EcmaScriptDependencyResolver.resolve (file:///C:/Users/kgetz/AppData/Local/npm-cache/_npx/c24e6b3ef94a6d87/node_modules/skott/dist/src/modules/resolvers/ecmascript/resolver.js:87:40)
    at Skott.collectModuleDeclarations (file:///C:/Users/kgetz/AppData/Local/npm-cache/_npx/c24e6b3ef94a6d87/node_modules/skott/dist/src/skott.js:191:47)
    at async Skott.buildFromRootDirectory (file:///C:/Users/kgetz/AppData/Local/npm-cache/_npx/c24e6b3ef94a6d87/node_modules/skott/dist/src/skott.js:279:13)
    at async Skott.initialize (file:///C:/Users/kgetz/AppData/Local/npm-cache/_npx/c24e6b3ef94a6d87/node_modules/skott/dist/src/skott.js:287:13)
    at async skott (file:///C:/Users/kgetz/AppData/Local/npm-cache/_npx/c24e6b3ef94a6d87/node_modules/skott/dist/index.js:8:27) {
  code: 'ERR_INVALID_ARG_TYPE'
}
kaiyoma commented 1 year ago

Another issue: even after skott exits, some Node process keeps running in the background and chewing up a ton of CPU until I kill it from the task manager. Seems kind of related to the other problems I'm seeing.

antoine-coulon commented 1 year ago

Hello @kaiyoma,

Thanks for the information. Using the latest stack trace, I was able to check what piece of code was throwing that error, but I'm not able to understand why this happens using the configuration you just provided me. Here is a reproduction of the setup using the tsconfig mentioned above using StackBlitz and when running npm run skott you can see that it successfully builds the graph using the path aliases.

I'm planning to implement a verbose mode to log everything happening under the hood. Having a bit of context + few operations before the crash will help me for sure determine the root cause hence implementing a fix.

Meanwhile what I can do is implementing blindly a unexpected error handler at the precise location according to the stack trace. It won't fix the problem, it will just ignore the module declaration that is involved in that error and end the traversal for it hence the graph might miss some of the sub-project internal nodes but in the context of Rush.js you should not care much as internal nodes are only used to determine third-party and built-in dependencies that are used within a project (using the EcmaScriptDependencyResolver that seem to be the problem).

The only ones that are important for now are the workspace module declarations of the monorepo (and that should be working fine), not the TypeScript path aliases. These dependencies are tracked using the RushDependencyResolver. Indeed what I could even do faster than that is providing a flag to disable the EcmaScriptDependencyResolver and only use the RushDependencyResolver allowing you to build the Rush monorepo graph. As I said it won't provide you whole the metadata that's found by the EcmaScriptDependencyResolver (third-party modules, built-in modules) but maybe you don't even care about that.

TL;DR:

  1. I'll publish a version of the rush-plugin-skott with the ability to only use the RushDependencyResolver (it makes sense to have that option even out of the scope of this issue).
  2. I'll add things to facilitate the troubleshooting on skott's side to deal with EcmaScriptDependencyResolver unexpected/edgy problems.
  3. Please check the StackBlitz linked above if you have time, to tell me if you have another way of using the path aliases.
kaiyoma commented 1 year ago

Our repo is quite large and complicated, and actually includes both monorepo and non-monorepo elements, so I'm not surprised that you can't repro this problem easily on your side. I think adding verbose logging would be the best path forward here and I'm happy to gather those logs for you and report back.

Another interesting data point: the stack trace appears to be happening only on Windows. rush graph completes successfully on Linux and macOS, but only after deleting the Docusaurus output directory. If I don't delete that directory, rush graph and npx skott both hang indefinitely on every OS.

antoine-coulon commented 1 year ago

I think adding verbose logging would be the best path forward here and I'm happy to gather those logs for you and report back.

I'll do that as a priority for sure. Next week I'm off, so won't be able to do it until I'm back.

I think adding verbose logging would be the best path forward here and I'm happy to gather those logs for you and report back.

Thanks for offering that help, it's still the early days for the skott ecosystem and there is a lot to do so I truly appreciate that you're taking the time to report the issue and help me improving the whole thing.

Another interesting data point: the stack trace appears to be happening only on Windows. rush graph completes successfully on Linux and macOS, but only after deleting the Docusaurus output directory. If I don't delete that directory, rush graph and npx skott both hang indefinitely on every OS.

That's indeed an interesting input! The .docusaurus problem will be solved by making skott ignore what must actually be ".gitignore"-d, there is already an on-going PR that will solve the problem. It would not amaze me to see that skott is stuck in a infinite loop analyzing this type of files. I'll take a look at it though.

For the cases where rush graph completed (macOS/Linux), did it work as expected for you?

Putting aside the very experimental status of this plugin hence the bugs and the very limited API as of now, I'd be happy to hear your suggestions/things that would be valuable from your experience that could be implemented in the future. I already had some feedback from the Rush core team and I already have many things to do in my internal backlog, I'll try to make them public in this repo. Consequently if you prefer to talk about suggestions feel free to do it wherever it's the more convenient for you (here, other issue on this repo, using the thread on the Rushstack issue...)

kaiyoma commented 1 year ago

For the cases where rush graph completed (macOS/Linux), did it work as expected for you?

I still can't get it to complete locally (I have a Windows 11 laptop), but my worker who has a MacBook seems pretty happy with the output! I've seen some screenshots from him and it looks like skott is correctly mapping out the dependency graph of all our packages. I'm hoping to incorporate this into our (Linux-based) CI so that we can generate and publish a new graph on every merge. It'll be really helpful - especially for newbies - to have a visual reference for the monorepo that's always up to date.

antoine-coulon commented 1 year ago

@kaiyoma Alright thanks for the feedback! It's only the beginning and there is still a lot to do so if you have any use case in mind that could be valuable for you feel free to open another issue with the feature request :)

antoine-coulon commented 1 year ago

Hey @kaiyoma hope you're doing well. Just published skott@0.22.0 including a "--verbose" flag that can be used to display a bunch of logs. Not everything is logged yet but it might be helpful to try it out.

FYI I'll release soon a new version of the plugin that does exactly the same thing and expose the same flags so that we avoid passing by skott directly

kaiyoma commented 1 year ago

Here's the output after letting it run for a few seconds: skott.txt

I see this error right away:

 ✖ An exception occured reading from tsconfig: tsconfig-geiger/tsconfig.json. Reason ENOENT: no such file or directory, open 'C:\Users\kgetz\Work\event-viewer\tsconfig-geiger\tsconfig.json'} 

That path is not quite correct. Our tsconfig has been factored out to a package, and skott doesn't seem to be resolving that correctly. Our root tsconfig has this:

"extends": "tsconfig-geiger/tsconfig.json",

However, that is linked in our node_modules via pnpm's linking mechanism, like this:

"tsconfig-geiger": "link:configs/tsconfig-geiger",

Since this seems to be a Windows-only issue, maybe the backslashes in the path are also causing issues?

antoine-coulon commented 1 year ago

Thanks for the quick feedback once again!

That path is not quite correct. Our tsconfig has been factored out to a package, and skott doesn't seem to be resolving that correctly. Our root tsconfig has this: "extends": "tsconfig-geiger/tsconfig.json",

This is actually expected because skott does not handle external (non-relative) tsconfig "extends" yet, and I actually thought of supporting lately, that's why I created that issue few days ago 😄 This is actually pretty easy to solve, so it will be done very quickly.

Nevertheless, this won't actually solve your problem while using the plugin for Rush.js as resolving Rush dependencies doesn't rely on the ECMAScript dependency resolver (in charge of resolving TypeScript path aliases) but on the Rush resolver embedded in the plugin (and that will be available soon to use without any other resolver)

Since this seems to be a Windows-only issue, maybe the backslashes in the path are also causing issues?

Yeah it's probably a Windows specific problem, but backslashes should be fine because I'm using the cross platform path API. Even if that was that, I doubt that this would prevent the process from finishing, at worse it could just ignore some files resulting in missing graph nodes. I need to investigate and try to reproduce that on my end using a Windows computer.

Moreover I downloaded the file you shared containing the logs and from what I can see at the end of file the process keeps resolving modules without finishing, was the file truncated or did you exited the process manually to avoid the Runs forever and never finishes that you described me before? I just want to be sure that we're still talking about that "never ending" problem.

FYI I'm already very grateful for the time you're taking to precisely report the problem so I wanted to let you know that I'll stop any feature enhancement and make this problem resolution a priority by switching up fully to Windows as it's probably related to that somehow, it will be easier for me to reproduce that on my own.

kaiyoma commented 1 year ago

I ran it again and let it run to completion: skott-verbose.txt