-
Notifications
You must be signed in to change notification settings - Fork 556
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
feat: check that there are actually modules in node_modules #128
Conversation
Travis CI seems broken - it fails because snyk is not authorized. |
@jehy thanks for this contribution! Overall a very clean implementation! I'm slightly concerned with this flow:
When using Can you suggest a way to overcome this? I'd attempt to run It would also be great to see tests added for this scenario - see https://github.com/snyk/snyk/blob/master/test/missing-node-modules.test.js#L11 for a relevant test. Thanks for your effort, it is much welcome! I'd like to help seeing this through :) |
I don't think that it's right to test project with zero dependencies, but I understand that breaking changes are pretty bad. In https://github.com/snyk/snyk/issues/126 I described why relying on npm version is unreliable. But I added another commit to this pull request - it checks whether all dependencies from |
On January 25, I added this PR. It think that snyk developers should close pull requests section on github if they are are not going to work with them. |
Closing this as the issue is resolved as per confirmation on the issue: https://github.com/snyk/snyk/issues/126 |
What does this PR do?
This checks that there are actually some modules in node_modules directory.
I also added a ToDo because it is better to check that all deps from package.json are installed.
Where should the reviewer start?
From the beginning, of cause.
How should this be manually tested?
There can be three cases:
Any background context you want to provide?
This will break testing projects which have zero dependencies and have an empty
node_modules
directory but I don't think that this testing makes any sense - it is clearly a false positive.What are the relevant tickets?
https://github.com/snyk/snyk/issues/126
Screenshots
Additional questions