cvisco / eslint-plugin-requirejs

Enforce code conventions for RequireJS modules with ESLint
MIT License
29 stars 16 forks source link

New rule: no-unused-dependencies #75

Open afahim opened 8 years ago

afahim commented 8 years ago

Do we have a rule that checks if all the dependencies required are actually utilized within the file or not?

For instance, for a file with such contents

define([
    "underscore",
], function(
    _,
) {
    console.log("hi");
});

It would say something like 'warning: you have required underscore but aren't actually using it.'

cvisco commented 8 years ago

@afahim ESLint already has a no-unused-vars rule that accomplishes this.

platinumazure commented 8 years ago

Between no-unused-vars and requirejs/amd-callback-arity (which checks that you don't have too many or too few callback parameters compared to the number of dependencies you've required), I think your use case is probably covered. Try those out and let us know if you think there's a gap somewhere!

afahim commented 8 years ago

The issue is that by default, no-unused-args only checks if the last argument is being used or not. Unless someone specifically specifies that all args should be checked (instead of the default after-used option), only the last dependency is checked for and erred upon.

afahim commented 8 years ago

Here is a case where no-unused-vars doesn't detect any unused imports.

define["backbone", "underscore", "moment"],
function(backbone, underscore, moment) {
    console.log(moment);
}
cvisco commented 8 years ago

@afahim got it. I've never run into this particular case before since I tend to use the Simplified CommonJS Wrapper style. With that form, each unused dependency is flagged appropriately. The ESLint no-unused-vars rule's behavior makes good sense in every function except for AMD callbacks. :disappointed:. So, if there is no way to achieve this particular case with existing ESLint rules, then this looks like a valid addition.

cvisco commented 8 years ago

I'm curious where the warning should be placed, though. On the callback argument or on the path in the dependency array? Any thoughts?

afahim commented 8 years ago

@cvisco yes, we tried quite a few different approaches to enforcing this in our codebase, but since we just use the regular RequireJS way of imports in many different parts of the codebase, none of the existing rules worked :(

We also wanted to add a optional whitelist because we realize that some modules need to be loaded because of their side effects, but they don't necessarily have to be consumed within the module (this specifically applies to writing client side code).

With that in mind, I was thinking the warning should mention the path and not the callback argument. Let me know if you have any other thoughts!

cvisco commented 8 years ago

@afahim makes sense to me!

captbaritone commented 8 years ago

:+1:

obrejla commented 8 years ago

To me as well ;) One can them move such "side effect modules" at the bottom of the AMD array and do not include their names in AMD function param list.

+1

afahim commented 8 years ago

I see that some of the functionality we will need to write this rule is already written as helper functions in the eslint no-unused-vars rule. What would you guys say to calling some of those helper functions to write this rule? If you think it's too brittle we can try to mimic the logic here instead.

platinumazure commented 8 years ago

No, don't call them directly- rules shouldn't know about each other. You can copy the functions for now, but we should open an ESLint issue to see if they recommend perhaps extracting those functions to a utility we could use.

sbason commented 8 years ago

Hi - where did we get to with this rule? Did anyone start writing any code?

@afahim If you're still interested in a whitelist for modules loaded for their side effects I added this feature to amd-function-arity recently - https://github.com/cvisco/eslint-plugin-requirejs/issues/77

cvisco commented 8 years ago

@sbason, I don't believe anyone has started work on it yet. If you'd like to take it, feel free!

schallm commented 7 years ago

I am looking for a tool that can check for this exact issue. I have not used eslint at this point, but it seems like a great tool. Is this feature still planned?

cvisco commented 7 years ago

@schallm Unfortunately, due to my current schedule, I won't be able to get to this feature any time soon, but I am always open to pull requests if someone else would like to tackle it.

obrejla commented 7 years ago

I think that I'll look at it...just not sure when ;)

TobiasHennig commented 6 years ago

👍

obrejla commented 6 years ago

Still not sure when because of job change and daughter born...:/

rahulrana95 commented 6 years ago

Anyone working on this?

rahulrana95 commented 6 years ago

@cvisco

cvisco commented 6 years ago

@rahulrana95 No one is currently working on this. Feel free to submit a pull request for it, if you'd like, though!