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

Symlinked files are ignored #256

Closed
magillj opened this issue Jul 2, 2018 · 9 comments · Fixed by #347
Closed

Symlinked files are ignored #256

magillj opened this issue Jul 2, 2018 · 9 comments · Fixed by #347

Comments

@magillj
Copy link

magillj commented Jul 2, 2018

depcheck does not process source files that are symlinked to a file outside the package being analyzed.

This is simple to repro:

  1. Create a new package with a blank package.json
  2. Add a js file where the only line is var test = require('lodash');
  3. Running depcheck on that package properly shows that the dependency lodash is missing
  4. Move the js file somewhere outside the package and symlink to it inside the package
  5. depcheck reports no issues

I found the problematic code: only regular files (the type 'file') are checked. This group does not include the 'link' category, which contains the symlinks in question. That's why they're ignored.

Context: the reason I have to worry about symlinks is that I'm using this within a bazel build system, which symlinks groups of source files to sandbox environments where tests run.

@mnkhouri
Copy link
Member

Hi @magillj, thank you for the detailed issue report! The module we're using to enumerate files, walkdir, supports an option to follow symlinks. This option has been off in this project since the very first commit!

I don't see a reason why we couldn't turn this option on 🤔

A PR with this change as well as a test to cover it would be very welcome :) might you have time to take a shot at a PR? If not, no worries, I'll try to get to it soon

@magillj
Copy link
Author

magillj commented Aug 31, 2018

Sure I might be able to give it a shot within the next couple weeks

@mzyil
Copy link

mzyil commented May 9, 2019

any updates on this?

@rumpl
Copy link
Member

rumpl commented May 9, 2019

@mzyil Not really no. Is your use case the same as @magillj ?

If not, could you tell me what is your use case?

rumpl added a commit that referenced this issue May 9, 2019
Adding a test for:
- file symlinks
- directory symlinks

Fixes #256
rumpl added a commit that referenced this issue May 9, 2019
Adding a test for:
- file symlinks
- directory symlinks

Fixes #256
@rumpl
Copy link
Member

rumpl commented May 9, 2019

So yeah, I have an update on this. OF COURSE SYMLINKS ARE A PITA ON WINDOWS!

😢

Will try to find a solution.

@rumpl rumpl closed this as completed in #347 May 9, 2019
@rumpl
Copy link
Member

rumpl commented May 9, 2019

Done, will be available in the next release of depcheck.

Sorry it took so long, real life got in the way :)

Cheers

@mzyil
Copy link

mzyil commented May 9, 2019

Thanks for the update! There is one more feature that we would like, will open another issue about that soon.

However may I suggest you make the following symlinks feature optional? Not our use case per se, but you may want to honor NODE_PRESERVE_SYMLINKS env var for example, in very rare cases one may have generated files in the repository that they don't care.

@rumpl
Copy link
Member

rumpl commented May 9, 2019

Oh, I didn't know about NODE_PRESERVE_SYMLINKS, you learn every day :)

Would you like to make a PR for this? This would be a great time to drop node 6 support too (NODE_PRESERVE_SYMLINKS was added in v7.1.0).

@mzyil
Copy link

mzyil commented May 9, 2019

I'm planning to open a PR anyway, I will see what I can do

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

Successfully merging a pull request may close this issue.

4 participants