Skip to content

Conversation

david-zacharias
Copy link
Contributor

As described in #186 it seems the plugin marks methods on chain with prefer-lodash-method error when chain is imported as member import { chain } from 'lodash'.

This PR fixes this by checking module imports for chain, registering this as general module, and check if a chain() call refers to the import from lodash.

Unfortunately I was not able to cover this change with a test because it requires to test if chain is an imported member from lodash. Hints how to cover this welcome!

@coveralls
Copy link

coveralls commented Oct 10, 2018

Pull Request Test Coverage Report for Build 407

  • 33 of 33 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 401: 0.0%
Covered Lines: 1864
Relevant Lines: 1864

💛 - Coveralls

Copy link
Contributor

@ganimomer ganimomer left a comment

Choose a reason for hiding this comment

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

Great job and thanks for contributing!
Two comments, though:

  • You added the chain if it's imported in an ImportDeclaration. You should add this logic for the other case of VariableDeclarator for cases like const {chain} = require('lodash').
  • You can add tests in the LodashContext test file. If you take this test for example, you can see it's very similar to what you'd need to do.

Thanks for getting into the internals and fixing this.

@david-zacharias
Copy link
Contributor Author

You are welcome, thanks for the testing hints!

I have updated my PR with two more commits:

  1. Relates to your feedback and adds tests as well as covering the case of requireing via VariableDeclarator
  2. I noticed that isImplicitChainStart returned true for chain()... leading e.g. to wrong removal of .value() by no-double-unwrap when used with --fix. My second commit tries to cover that.

@david-zacharias
Copy link
Contributor Author

@ganimomer May I ask if you had time to take a look at the updated code? Anything I should change?

@ganimomer
Copy link
Contributor

Sorry for taking long to reply!
This looks good. The only thing missing is adding a test in prefer-lodash-method that the original error doesn't reproduce anymore.

@david-zacharias
Copy link
Contributor Author

david-zacharias commented Nov 13, 2018

Now I have to say sorry for my late reply. I added a test that (from my perspective) covers the originating problem (failed without changes of this PR, runs green with the changes).

@ganimomer ganimomer merged commit e32432b into wix-incubator:master Nov 21, 2018
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.

3 participants