wix-incubator / eslint-plugin-lodash

ESLint rules for lodash
MIT License
276 stars 65 forks source link

Rule idea: Valid lodash method #36

Open jkrems opened 8 years ago

jkrems commented 8 years ago

I'm currently migrating from lodash 3 to lodash 4. I looked into this plugin to help me identify the parts of my code where I'm using lodash methods that no longer exist (e.g. pluck). Unfortunately with all checks turned on I get the following eslint errors:

/.../test/cluster.test.js
  46:9  error  Prefer _.startsWith instead of comparing indexOf() to 0  lodash/prefer-startswith

But the tests fail with:

  1) cluster interactions logs worker exits:
     TypeError: _.pluck is not a function
      at Context.<anonymous> (test/cluster.test.js:36:23)

Would it be worthwhile to add a rule that fails when you use an unknown lodash method? Or am I missing an existing one?

ganimomer commented 8 years ago

Hi, Thanks for the idea, but I don't think this is worthwhile. For one thing, this type of error can be caught pretty easily (you just need to reach this code in the tests, not even test its functionality - like you just showed). And also, with _.mixin you can generally add any method you want to the _ object and this rule would have false positives.

urbackmk commented 8 years ago

darn, I'm also upgrading lodash versions and was hoping that this plugin would catch unknown lodash method errors. I would agree with @jkrems that such functionality would be useful. It's easy to customize rules to be error/warn/off if the rule doesn't work with a specific codebase.

jfmengels commented 8 years ago

@urbackmk You might want to try https://github.com/jfmengels/lodash-codemods to help with your migration.

Back to the proposed rule, I actually think that it has its worth, if only for linting in the editor (quick feedback is very helpful), to catch simple typos for instance. Unknown Lodash method _.fidn. I would not necessarily go for telling the name of the method that replaced it if it's a method that existed, though that could be helpful.

For the _.mixin, you could either tell users to not use the rule when using mixins (but it could still help others), or add a setting to allow exceptions that they will have to maintain with their use mixins.

I'm thinking about adding it to eslint-plugin-lodash-fp, so if you have more reasons against it, would be happy to hear about them.

ganimomer commented 8 years ago

You make some solid points. Also, since the plugin contains an extensive list of all methods (and their maximum arities but that's besides the point), implementing this should be fairly trivial. However, this would also put a bit more pressure into keeping track of Lodash versions - since the list of methods is curated inside the plugin, any new method by Lodash would have to be added to the plugin pretty soon, but on the other hand, this is true whenever Lodash adds another argument to any of its existing functions, so that's not terrible.

Final verdict - this issue is reopened and should be included for work in the next version.

jfmengels commented 8 years ago

However, this would also put a bit more pressure into keeping track of Lodash versions

An other way to do it would be to require lodash and check if the wanted method exists inside it. I'm not sure how that would pan out, as I'm not sure that the imported Lodash is the same one as the one that the examined codebase would think of.

That's pretty much what I do in eslint-plugin-lodash-fp, as the only reliable source of lodash/fp mappings is inside the package (and changes a lot more and a lot more silently than vanilla Lodash). It works fine, but I only have to handle one version of Lodash (at the moment).

woutervanvliet commented 7 years ago

For a quick and dirty, one time use only, you can hack the prefer-alias rule by adding all deleted/removed methods as an alias of a nonexisting method.

Add the following to the bottom of /node_modules/eslint-plugin-lodash/lib/util/methodDataByVersion/4.js

    DELETEDMETHOD: {
        "aliases": [
            "support",
            "callback",
            "indexBy",
            "modArgs",
            "pairs",
            "pluck",
            "restParam",
            "sortByAll",
            "sortByOrder",
            "where",
            "backflow",
            "collect",
            "compose",
            "methods",
            "object",
            "select",
            "unique",
            "findWhere",
            "padLeft",
            "padRight",
            "trimLeft",
            "trimRight",
            "trunc",
            "all",
            "any",
            "contains",
            "detect",
            "foldl",
            "foldr",
            "include",
            "inject"
        ],
        "wrapper": false,
        "shorthand": false,
        "chainable": false,
        "iteratee": false
    }

It's about as ugly as can be, but very useful during migration ;)