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

Add verbose option #123

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add verbose option #123

wants to merge 2 commits into from

Conversation

extronics
Copy link

In my use-case i'm processing the files, identified by node-dependency-tree, again. In order to avoid re-resolving the dependencies in my own code, i added the partial to the output. This way, it's easy to identify which syntax node in a babel AST refers to which dependency.

The format i choose is easy for extending with further internals of node-dependency-tree, if need arises (see README.md)

I was not able to run the tests - many of them failed without my changes already. Are they supposed to be functional/can you give me some pointers what's needed to get them running?

If you like the change i'd also add some tests if i can get them to run in the first place.

@mrjoelkemp
Copy link
Collaborator

Hey @extronics! Thanks for contributing. I like the idea a lot.

Just thinking out loud a bit, but I was curious if it would be easier to just have this verbose mode be the internal, default data structure and then have the existing api methods re-traverse that and pluck out the resolved value. That will minimize forgetting to support verbose mode in all of the places, the tradeoff is it will be slower re-traversing the data structure to form the output. Or we could use more space and store two representations in memory (the verbose one and the simpler one), keeping the performance roughly the same.

Do you think the maintenance worry of forgetting about verbose mode's alternate structure is a real concern? Or is it unlikely that we'll need to really touch this moving forward?

@extronics
Copy link
Author

extronics commented Nov 9, 2020

@mrjoelkemp good point, this would solve some of the config.isListForm ? [] : {} pattern :)

I think the performance penalty won't be a huge concern as long as the re-traversal while building the output also re-uses duplicate sub-trees (https://github.com/dependents/node-dependency-tree/pull/123/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346L144). From my observations, the really huge dependency trees originate from source files which include some top-level file of their own codebase again.
E..g. somewhere deep in react you see some pattern like import * as React from 'react' which just repeats the full dependency tree of react again and again.
So as long as the re-traversal stops early as soon as it crosses these duplicate sub-trees we should be fine.

At least from my observations. Ive seem some dependency trees in popular libraries in the range of 50 million items - almost all of them from duplicate sub-trees though. Skipping those made for sub-millisecond traversal still because you end up visiting a few dozen unique nodes only.

That being said - i known that there is a lot of obscure stuff going on in real life code bases so i might be missing some reasons to avoid re-traversal :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants