dependents/node-dependency-tree

Support for exposing non-existent paths

j-lee opened this issue · 9 comments

j-lee commented

First off, thanks for developing this great module! It was actually kind of difficult to find much options out there of being able to get a JS paths dependency tree, but this one worked wonders and not as limited as the requirejs optimizer.

What I'm trying to solve now is, during the build/test process of our app I'd like to be able to identify in the dependency tree paths to files that do not exist. From what it appears, dependency-tree (file-cabinet?) completely excludes paths to non-existent files from the output. I'd like to be able to detect for non-existent paths during our build/test process so we can to catch these errors in advance and not at runtime. Is there any way that dependency-tree can handle this somehow? Or is there another way/suggestions to achieve this?

Thanks!

Hey @j-lee! Thanks for the kind words and for your question.

Support for a new object, named nonExistent (similar to visited) could be added that will collect all of the non-existent paths found during traversal.

  • It would be added to Config:
    this.visited = options.visited || {};
  • It should be documented in the doc block:
    * @param {Object} [options.visited] - Cache of visited, absolutely pathed files that should not be reprocessed.
  • You can register the non-existent result here:
    debug('filtering non-existent: ' + result);
  • Consumers that supply nonExistent could then do Object.keys(nonExistent) to get the unique paths to module paths that could not be found.
    • It's also plausible that filing-cabinet could (incorrectly) map a partial/dependency path to an incorrect filepath. It would be great to get bug reports if that's the case. In the terminal, you can see some helpful debug output by running a command like DEBUG=* dependency-tree path/to/my/file.js; please provide that in your report.
  • There should be tests to assert that dependency tree collects the non existent files when given a nonExistent storage object.

An alternative here is to support a flag that includes nonExistent files in the generated tree. However, you wouldn't be able to discern (without a second pass that checks for the existence of every key in the generated tree) which files are the non-existent ones.

Would you be open to submitting a PR for this?

j-lee commented

Hi @mrjoelkemp, thanks for the quick reply, and the specific details for where to introduce this! Interesting that most of it is there to supporting this relatively easily, and hopefully filing-cabinet plays nice. I'll give this a shot and see how it goes!

To ensure that filing-cabinet plays nicely, we'd have to look through all
of the revolvers and see if they return an empty string if they didn't
resolve to a path that exists.

It might be helpful to include the partial name in the nonExistent object.
This way, if a partial resolved to an empty string, we have some idea of
which partial was problematic. Thoughts?

On Fri, Sep 2, 2016, 2:45 AM j-lee notifications@github.com wrote:

Hi @mrjoelkemp https://github.com/mrjoelkemp, thanks for the quick
reply, and the specific details for where to introduce this! Interesting
that most of it is there to supporting this relatively easily, and
hopefully filing-cabinet plays nice. I'll give this a shot and see how it
goes!


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#46 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA0zsz3RO4M5ESU9P8P_qrvTrL9D3aiGks5ql8XqgaJpZM4JzGnY
.

j-lee commented

At the moment, it looks like filing-cabinet already returns an empty string when the partial doesn't resolve, so I take it that it's okay and we can just do this without touching filing-cabinet?

I was thinking maybe nonExistent could be an array of objects, where each object contains properties:

  • partial - the full path to the non-existent partial [path.join(config.directory, dep)]
  • parent (?) - the full path from where the non-existent partial is being called from [config.filename]

So you have both the bad partial and where the bad partial is being depend/require'd from.

Though this "list of objects" wouldn't quite follow any output pattern this module already uses...

Otherwise, maybe something a bit more consistent, but not sure how to set up in code yet:

{
   "path/to/parent/file.js": {
        "/path/to/bad/partial1",
        "/path/to/bad/partial2"
    }
}

Thanks so much for your thoughts!

My original idea was to avoid duplicates in the nonExistent object. With
your proposed list of objects, we'd have duplicate entries when a partial
is referenced from multiple parents.

I like your second suggestion, though, it doesn't prevent duplicate
partials across multiple parents. I also don't know if providing all of the
parents is necessary since you could search your codebase for the
nonExistent partials and find all of their parents.

I'd also be careful of saying that the partial's full path is the concat of
the directory and the partial since AMD and webpack support partial name
aliasing. Using only the raw partial path is less misleading.

On Fri, Sep 2, 2016, 2:46 PM j-lee notifications@github.com wrote:

At the moment, it looks like filing-cabinet already returns an empty
string when the partial doesn't resolve, so I take it that it's okay and we
can just do this without touching filing-cabinet?

I was thinking maybe nonExistent could be an array of objects, where each
object contains properties:

  • partial - the full path to the non-existent partial [path.join(config.directory,
    dep)]
  • parent (?) - the full path from where the non-existent partial is
    being called from [config.filename]

So you have both the bad partial and where the bad partial is being
depend/require'd from.

Though this "list of objects" wouldn't quite follow any output pattern
this module already uses...

Otherwise, maybe something a bit more consistent, but not sure how to set
up in code yet:

{
"path/to/parent/file.js": {
"/path/to/bad/partial1",
"/path/to/bad/partial2"
}
}


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#46 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA0zs6LSA_Lk21cViPvgNfBZn9y3ElVNks5qmG7_gaJpZM4JzGnY
.

j-lee commented

Makes sense about the path concat. Thanks for the additional context from other config usages.

So if we end up only using the raw partial path and not need the parents, assuming people can just search for the broken partial paths, maybe nonExistent can just be an array of partial paths (array of strings), and without dupes?

I was hoping to avoid the quadratic runtime of deduping arrays, but it's
not a deal breaker since it only really affects large numbers of non
existent partial resolutions. Let's do it. If you want to submit an early
PR without tests as a proof of concept, I'll gladly review it.

On Fri, Sep 2, 2016, 7:18 PM j-lee notifications@github.com wrote:

Makes sense about the path concat. Thanks for the additional context from
other config usages.

So if we end up only using the raw partial path and not need the parents,
assuming people can just search for the broken partial paths, maybe
nonExistent can just be an array of partial paths (array of strings), and
without dupes?


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#46 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA0zs-Azch2Jwn6DHJnJKm2LiSwip3fFks5qmK7PgaJpZM4JzGnY
.

j-lee commented

Hi @mrjoelkemp, I've referenced a commit from my forked repo. Could you take a look at it? Thanks!

Hey @j-lee. I opened a PR with your changes and left some comments! Thanks again.