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

Remove unused feature #166

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

sholokhov
Copy link

Hi!
Recently I opened issue #165 about automatically removing unused dependencies via special option. After some code exploration I didn't find this feature but I think that it will be quite convenient for end users. So I decided to add it into code base.

@sholokhov sholokhov changed the title Remove unused issue #165 Remove unused feature Aug 2, 2016
function removeUnused(currentState) {
const dependenciesToRemove = currentState.get('unusedDependencies');
if (skipAutoRemove(currentState) || _.isEmpty(dependenciesToRemove)) {
return new Promise(resolve => resolve(currentState));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return Promise.resolve(currentState)

@LinusU
Copy link
Collaborator

LinusU commented Aug 2, 2016

Nice, I'll review this properly when I get the time :)

@sholokhov
Copy link
Author

@LinusU thanks, I'm going to fix remarks after your full review :)

@sholokhov
Copy link
Author

Hi @LinusU, any updates on this?

@amilajack
Copy link
Collaborator

amilajack commented Oct 7, 2016

Please fix this conflicts in this branch so it can be merged.

@sholokhov
Copy link
Author

@amilajack ok, I've fixed it.

@amilajack
Copy link
Collaborator

After commenting that, I found out that this project doesn't have very comprehensive testing support. Have you tested this locally? Reluctant to merge until we have some kind of e2e test so we don't break the module.

@MaffooBristol
Copy link

Thanks for this. Does this allow removing unused from npm-check -u too? It doesn't look like it from the PR, would be nice to add it as a sub-section!

@sholokhov
Copy link
Author

sholokhov commented Oct 13, 2016

@amilajack I also did not found any auto tests for this project, but I ran it locally on my several projects. So on my local environment it works well and doesn't affect any other functionality of npm-check. For a greater certainty one can fetch this branch out and try it on some testing projects until tests will be added to the project.

@MaffooBristol no, it doesn't allow you to remove unused dependencies via npm-check -u and actually I think that it is not necessary. You can just combine these two options if you want to upgrade and remove dependencies simultaneously (which is a classic Unix-way ;)

@wmertens
Copy link

@sholokhov actually I get quite a bit of false positives for unused imports - I would rather have a section in npm-check -u so I can decide for myself. It's impossible to be 100% correct about unused imports.

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

Successfully merging this pull request may close these issues.

5 participants