Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kaiyoma opened this issue Jul 20, 2023 · 14 comments
Labels
bug Something isn't working core discussion Discussion about anything like a feature request

Comments

@kaiyoma
Copy link

kaiyoma commented Jul 20, 2023

I have the latest version of this tool installed, but it crashes every time I run it:

$ skott --version
skott, 0.28.0

 Skott exited with code 0

$ skott -s

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

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
    at new NodeError (node:internal/errors:399:5)
    at validateString (node:internal/validators:163:11)
    at Object.join (node:path:429:7)
    at resolveAliasToRelativePath (file:///C:/Users/kgetz/AppData/Roaming/nvm/v18.16.0/node_modules/skott/dist/src/modules/walkers/ecmascript/typescript/path-alias.js:10:36)
    at resolvePathAlias (file:///C:/Users/kgetz/AppData/Roaming/nvm/v18.16.0/node_modules/skott/dist/src/modules/walkers/ecmascript/typescript/path-alias.js:88:35)
    at EcmaScriptDependencyResolver.resolve (file:///C:/Users/kgetz/AppData/Roaming/nvm/v18.16.0/node_modules/skott/dist/src/modules/resolvers/ecmascript/resolver.js:113:40)
    at Skott.collectModuleDeclarations (file:///C:/Users/kgetz/AppData/Roaming/nvm/v18.16.0/node_modules/skott/dist/src/skott.js:213:47)
    at async Skott.buildFromRootDirectory (file:///C:/Users/kgetz/AppData/Roaming/nvm/v18.16.0/node_modules/skott/dist/src/skott.js:285:13)
    at async Skott.initialize (file:///C:/Users/kgetz/AppData/Roaming/nvm/v18.16.0/node_modules/skott/dist/src/skott.js:305:13)
    at async skott (file:///C:/Users/kgetz/AppData/Roaming/nvm/v18.16.0/node_modules/skott/dist/index.js:27:27) {
  code: 'ERR_INVALID_ARG_TYPE'
}

Node.js v18.16.0
@antoine-coulon
Copy link
Owner

antoine-coulon commented Jul 20, 2023

Hello @kaiyoma, sorry to hear that it's still causing issues on your side.

During the time we have been discussing it on another issue which seems to be the same, unfortunately I was not able to reproduce the error during that time, hence the persistent issue. I added few error boundaries in #70 that should fix the runtime error even though it will not help resolving the underlying module (released in 0.28.1)

I remember that you provided the --verbose outcome in the past but as I added more fine-grained logs around path aliases in this PR it would be helpful to have logs with the new version (0.28.1).

Thanks

@kaiyoma
Copy link
Author

kaiyoma commented Jul 21, 2023

Ah, my apologies, I forgot about the other issue. Feel free to close this as a duplicate if you want.

Some good news is that if I drop into the src/ sub-directory in my repo, then the skott analysis completes and finds a whole bunch of import cycles. Great success! I think this will actually work nicely for us and we probably don't need anything more from you.

In the interest of data sharing, here's the full verbose output from running skott in our repo root: skott.txt

I had to kill the command after a couple minutes to keep the logfile small enough to upload.

@antoine-coulon
Copy link
Owner

Indeed it's my mistake, I originally wrongly moved it to the Rush plugin repo, so it's fine there.

Some good news is that if I drop into the src/ sub-directory in my repo, then the skott analysis completes and finds a whole bunch of import cycles. Great success! I think this will actually work nicely for us and we probably don't need anything more from you.

Happy to hear that! I'd say it's indeed a good practice to target a bit more precisely files to be included in the graph either by using --cwd (moving directly into the folder) or using --ignorePattern, skott is black-list oriented meaning that it might explore too many unwanted things if run at the root of many files.

In the interest of data sharing, here's the full verbose output from running skott in our repo root: skott.txt

Thanks for the file sharing, it will for sure provide me good leads for further investigation.

@antoine-coulon
Copy link
Owner

Also @kaiyoma, as you mentioned that you were using the web application, feel free to share feature suggestions as I'm currently revamping it from scratch (see RFC #72), in the case where you or your collaborators felt like things were missing from the application (or skott itself).

Thanks in advance

@antoine-coulon antoine-coulon added bug Something isn't working core labels Aug 28, 2023
@antoine-coulon
Copy link
Owner

Hello @kaiyoma, hope you're doing well.

Just wondering, are you still encountering the same set of problems? Also, don't hesitate to share me new information is you identified recurrent setups where skott was not working.

Thanks!

@kaiyoma
Copy link
Author

kaiyoma commented Sep 11, 2023

No new updates right now, but I'll report back if I discover anything new.

@antoine-coulon
Copy link
Owner

Alright thanks for replying that fast as always. I'm letting the issue opened for now so that we can use this communication channel if needed.

@antoine-coulon antoine-coulon added the discussion Discussion about anything like a feature request label Sep 12, 2023
@antoine-coulon
Copy link
Owner

Hello @kaiyoma, hope you're doing well :)

Lately I have been fixing few issues linked to Windows OS and as far as I can remember you had issues when running skott on this OS so if you're still using skott I would recommend you to jump on the latest version and try it out. You can then let me know how it goes and if you still encounter weird issues related to this topic

Thanks!

@kaiyoma
Copy link
Author

kaiyoma commented Dec 28, 2023

@antoine-coulon I still see issues running skott from the root of our repo; instead of crashing, the new version seems to just hang forever. However, I don't think you should worry about this, because we have an awkward "hybrid repo" that is both a single-repo and a monorepo at the same time. (It's a temporary stage as we transition from the former to the latter.) If I drop down into the source folder for our "single repo", the tool works correctly and identifies a whole bunch of circular imports that I need to fix. 😄

@antoine-coulon
Copy link
Owner

@kaiyoma great to know thanks. How many files does that hybrid repo contain? Does it hang forever or is it just very very slow? When running on few thousands files (less than 10k) it runs in a matter of seconds but when having too much files it unfortunately does not scale well performance-wise, up to several minutes according some tests I did. I'll soon include some benchmarks on pretty heavy codebases to provide me a fair foundation on top of which I'll be able to optimize and improve many things.

But given that it does not crash, we might consider the current issue being related to efficiency/performance and not compatibility.

@kaiyoma
Copy link
Author

kaiyoma commented Dec 28, 2023

@kaiyoma great to know thanks. How many files does that hybrid repo contain?

We have about 5,700 files in the "single repo" directory and about 11,400 files total.

Does it hang forever or is it just very very slow?

I'm not sure. I let it run for 5 minutes and it was constantly churning (with respect to CPU usage) and never finished. If I run skott on just the single source directory (the one with 5,700 files), it finishes in about a minute.

@antoine-coulon
Copy link
Owner

Thanks for the information :) This is more or less what I would expect given the current state of the tool.

It's also interesting to note that the number of files in itself is one parameter but then the number of relationships between files (that is graph edges) also come at a cost, so depending on the number of imports per module and the total number, the graph could be huge and slower to compute.

However it should never take more than few minutes for very heavy graphs imho, that's what I would expect if I was a skott user so improvements need to be done on that side.

Given you're very helpful as always I would like to take the opportunity to ask you one question: If I introduce a configurable flag to only and anonymously (code is fully open source) collect the time it took for skott’s graph (number of nodes and edges) to be computed to gather metrics and improve performance, would you enable it? If not, why? That data might be useful to track the performance path of skott.

Thanks!

@kaiyoma
Copy link
Author

kaiyoma commented Dec 29, 2023

If I introduce a configurable flag to only and anonymously (code is fully open source) collect the time it took for skott’s graph (number of nodes and edges) to be computed to gather metrics and improve performance, would you enable it?

Sure. 😄

@antoine-coulon
Copy link
Owner

Roger that, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core discussion Discussion about anything like a feature request
Projects
None yet
Development

No branches or pull requests

2 participants