Support `extends` #4

ta2edchimp · 2016-03-16T18:55:16Z

This Pull Request extends the interpretation of a given eslint configuration to take its extends field into account and also include the rules inherited thereby.
At the moment, it lacks the following:

proper tests (see further discussion)
.all-contributorsrc update
update documentation (README.md)

kentcdodds · 2016-03-16T18:57:03Z

bin.js

@@ -31,3 +34,12 @@ function getConfig() {
  }
 }

+function getRules(set) {
+  var configFileName = set ? ('eslint-config-' + set) : null


I wonder if we could find how ESLint resolves these extends because you can actually have files extend others like so. So we can either recreate the behavior or see if we can use ESLint's implementation somehow.

Oh yes, that's a good point. Didn't think of such a case at all ...

I investigated this a bit and came to the conclusion that we could use eslint itself to resolve all this for us.
That is, we could require('./node_modules/eslint/lib/config.js'), instantiate the exposed Config object and simply use its getConfig method, it will return an object with a property rules — from there on, it would be the same procedure that is already implemented.

But ...

We'd have to make eslint a (peer) dependency, which (in my opinion) would be just fine, as the whole purpose of this module is to work with it.

We'd have to require a part of eslint's internals that are not part of their public api, when used as a lib (as in require('eslint')).

Only drawback I can think of would be breaking when they change their internal architecture, concerning this certain part.

We'd have to make eslint a (peer) dependency, which (in my opinion) would be just fine, as the whole purpose of this module is to work with it.

Been meaning to do this anyway. 👍

We'd have to require a part of eslint's internals that are not part of their public api, when used as a lib (as in require('eslint')).

Maybe someone from the ESLint team could give us some insight on a better way to do this or at least give us an idea of how likely it is that this would lead to issues in the future. Cc @nzakas, @gyandeeps, @mysticatea, @ilyavolodin (top 4 code contributors on the project)

Sorry, I'm not sure I'm following exactly what you are trying to do, but I think you are looking for this: http://eslint.org/docs/developer-guide/nodejs-api#getconfigforfile This will automatically calculate all of the config values for you (including extends), and it's available as public API. I would suggest not using internals directly as they do often change.

How could we not see this ...
getConfigForFile() of the exposed CLIEngine object basically only wraps the functionality of the private api's Config.getConfig() m(
(I misinterpreted its documentation as only detecting the applicable rules/config of a particular [JavaScript] file based on its location within the filesystem.)

So, sth like

var path = require('path') var eslint = require('eslint') var rulesCombined = new eslint.CLIEngine().getConfigForFile(path.resolve('../path/to/config.file'))

(with ../path/to/config.file being the package.json or .eslintrc*) indeed seems to just do what we want.

I'll alter my code correspondingly later today or no later than tomorrow.

Hmm... the downside of getConfigForFile() is, when checking an eslint config outside of the own project's path it will inevitably break, because the path to lookup for extended configs will be reset to the own project's.

This would be no problem as long as eslint-find-new-rules is installed locally on a project's package and only used on configs that are contained within that project.
Installing eslint-find-new-rules globally and would not work, as referenced configs couldn't be found.

Installing eslint-find-new-rules globally and would not work, as referenced configs couldn't be found.

I'm not a fan of installing tools like this globally anyway, so that's fine.

What I'm worried about though is that doing this will utilize the ~/.eslintrc file which could contain extra rules (unless I'm mistaken). Also, in projects like mine where there are multiple files representing eslint config, I worry that having an .eslintrc file in there would be a problem... Am I misunderstanding it?

I was thinking a way to get around this would be to copy the config to a temporary directory on the filesystem and doing the check there... Thoughts?

Installing eslint-find-new-rules globally and would not work, as referenced configs couldn't be found.

I'm not a fan of installing tools like this globally anyway, so that's fine.

Ok, thought similar about this. I just wanted to be sure, we make the same assumptions here.

I thought of achieving the following:

Determine a particular eslint config als base

That is, when running eslint-find-new-rules without any argument: taking the currently applicable eslint config (from an .eslintrc file — or .js, .json, yml — or its corresponding property within the project's package.json).

When running eslint-find-new-rules with an argument: using that as particular eslint config file (even the corresponding property within a package.json, when specified as argument).

Collect all rules contained within the determined eslint config, as well as those from configs referenced to via extends.

Do not take any rules into account that may be applicable by considering the current location on the filesystem.

According to your example project:

When testing against index.js: collect only the rules within.

When testing against .eslintrc: collect all the rules within, extending the rules of react.js, extending the rules of index.js.

When we use the eslint api's method and there would be the sideeffect of accidentally taking environmental configs into account, that would also be the case when copying the file to test anywhere else on the filesystem (but within / somewhere below the same project root), because the api's method would look up all the way up to the project root ... at least this is as I currently understand its working behaviour. Or am I missing something?

My time's running out currently, and I have another appointment soon, but I'll have a closer look and run some more tests later tonight and tomorrow.

I just wanted to be sure, we make the same assumptions here.

We should probably document this limitation though, because other people make other assumptions :-)

Yeah, I think you've described the desired behavior. We'll probably run into some issues with those side-effects. But let's see how far we can get :-)

kentcdodds · 2016-03-16T18:58:50Z

On the testing side of things, I think we need to implement some tests for bin.js (#2) to get the coverage up. I'm willing to pull the standards down temporarily to get this implemented.

ta2edchimp · 2016-03-16T18:59:11Z

To catch up with the discussion from #1:

When adding another test to tests.js, like

test('provokes a memory leak warning', t => {
  const rules = [1, 2]
  t.same(rules, [1, 2, 3])
})

the following warning will be yielded:

(node) warning: possible EventEmitter memory leak detected. 11 test listeners added. Use emitter.setMaxListeners() to increase limit.

Edit
This also happens when completely commenting out the try...catch block requiring bin.js. (Like here)

ta2edchimp · 2016-03-16T19:16:43Z

Re lowering the standards temporarily: personally I'd rather hold this back for some time to see if we can get around the mentioned issue with the test and maybe also implement a more comprehensive resolution of the extends soon? Like, maybe until the end of the weekend?

kentcdodds · 2016-03-16T19:19:18Z

Sounds good to me!

kentcdodds · 2016-03-16T23:25:38Z

From @jamestalmage in gitter:

It’s related to your use of mock-fs. It interferes when AVA tries to load the source map. Use —verbose to see the real error. tschaub/mock-fs#62 would help solve it. In the meantime use proxyquire and mock.fs to avoid mocking the actual file system.

👍

ta2edchimp · 2016-03-17T17:40:55Z

I think from here, we have three options:

use the simple method introduced in this pr
use eslint's „private“ api
simply wait for a response to the issue/inquiry over at eslint's repo

That is, of course, only until there's a definite decision from the eslint team re considering exposing it's extends resolution, and then either use it or – if the proposal gets declined – „reproduce“ their way of resolving the extending rule configs.

Personally, I tend towards the 2nd option; it would at least make one comfortable with their way of doing it.

kentcdodds · 2016-03-17T20:47:32Z

@sarbbottam, I think you're referencing the wrong issue number in your commit message :-)

sarbbottam · 2016-03-17T20:59:45Z

Sorry about it, I fixed it.

Collect configured rules using the eslint api methods, taking extending configs and plugins into account.

codecov-io · 2016-03-19T10:47:56Z

Current coverage is `100.00%`

Merging #4 into master will not affect coverage as of 7bee015

@@            master      #4   diff @@
======================================
  Files            2       3     +1
  Stmts           22      37    +15
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit             22      37    +15
  Partial          0       0       
  Missed           0       0

Review entire Coverage Diff as of 7bee015

Powered by Codecov. Updated on successful CI builds.

ta2edchimp · 2016-03-19T10:51:32Z

@kentcdodds @sarbbottam anyone of you got an idea how we could test the support of plugins?
Or do we simply say, we don't explicitly need to test it, because we use an eslint api method and it should be covered by proper testing on their end?

ta2edchimp · 2016-03-19T11:00:04Z

Oh, and before merging, we'd have to make eslint a (peer) dependency.
I'd suggest

  "dependencies": {
    "eslint": "^2.4.0"
  }

to assure eslint >= 2.4.0 but < 3.0.0 is installed. But this may lead to a duplicate installation on environments with npm@2, so better make it a peerDependency?

What do you think?

ta2edchimp · 2016-03-19T13:07:16Z

I just realized, this pr will alter the behavior mentioned in the README, concerning defaulting to main: instead of using the script referenced in the package.json's main property as config to inspect, this pr's code will use the eslintConfig property.

@kentcdodds should I change this back to inspect the referenced main script instead? Or leave it as it is, inspecting the current package's eslint config (I'd alter the corresponding section within README.md in this case)?

kentcdodds · 2016-03-19T14:32:58Z

First, this is great! I think your suggestion on ESLint is good. But could we do ^2?

should I change this back to inspect the referenced main script instead? Or leave it as it is, inspecting the current package's eslint config

Which do you think is more intuitive. I built this originally for eslint-config- projects. But now I'm thinking a more wide audience would appreciate it because we can support extends and plugins...

ta2edchimp · 2016-03-19T15:32:12Z

So, I'll make it

  "peerDependencies": {
    "eslint": "^2.0.0"
  }

Regarding whether to use a package.json's main reference or the eslintConfig as default, I think we should stick with the original behavior (as some projects already use it this way: #14).
It should be documented though that, to use any package.json's eslintConfig property, eslint-find-new-rules can be pointed towards it directly.

kentcdodds · 2016-03-19T17:00:23Z

It should be documented though that, to use any package.json's eslintConfig property, eslint-find-new-rules can be pointed towards it directly.

There is a little documentation about that, but I agree :-)

kentcdodds · 2016-03-19T17:01:13Z

bin.js

    } else {
-      return require(path.join(process.cwd(), specifiedFile))
+      return path.join(process.cwd(), specifiedFile)
    }
  } else {
    // this is not being called with an arg. Use the package.json `main`


Should this comment be updated?

Well, I'm currently about to rewrite that part anyway (to restore the desired behavior described in the comment).

Cool, that's what I thought. Thanks!

kentcdodds · 2016-03-19T17:09:35Z

This looks awesome. I think there are just a few docs changes that we should add (like that we support extends and plugins etc.) 👍

ta2edchimp · 2016-03-19T17:14:30Z

Regarding plugins we still have an open issue that I planned to investigate further once I finished polishing this pr up: we somehow have to iterate through the rules possible by using certain plugins; atm we're only looking up eslint's very own lib/rules directory.

Restore the module's original behavior of interpreting the current package's main script as source of an eslint config, rather than taking the local package.json's eslintConfig property into account.

…ehavior

…be tested

…dency

ta2edchimp · 2016-03-19T18:41:16Z

@kentcdodds mind having a final look (esp. regarding changes in README, as I'm not a native speaker) and eventually merging or comment on anything you'd yet like to get included?

sarbbottam · 2016-03-19T18:47:28Z

.all-contributorsrc

+      "html_url": "https://twitter.com/ta2edchimp",
+      "contributions": [
+        "code", "test"
+      ]


You might want to add "docs" too.

kentcdodds · 2016-03-19T18:59:33Z

Other than the notes I've provided, this looks good.

kentcdodds · 2016-03-19T20:31:12Z

Looks good to me, and I think @sarbbottam approves as well. Let's do this!

Support `extends`

kentcdodds reviewed Mar 16, 2016
View reviewed changes

ta2edchimp mentioned this pull request Mar 17, 2016

This reproduces a bug in AVA (maybe?). Just run $ npm t and you'll … #6

Merged

kentcdodds mentioned this pull request Mar 18, 2016

Support plugin rules #3

Closed

feat(lib): Support extends and plugins

18b8aae

Collect configured rules using the eslint api methods, taking extending configs and plugins into account.

ta2edchimp force-pushed the support-extends branch from da46003 to 18b8aae Compare March 19, 2016 10:07

ta2edchimp added 3 commits March 19, 2016 11:32

test(tests): Test extending configs

cecadda

test(tests): Test extending configs

5a8859a

fix(lib): Assure comp. with Node 0.10

c1205f9

ta2edchimp force-pushed the support-extends branch from 33fc9f3 to c1205f9 Compare March 19, 2016 10:46

docs(contributors): added ta2edchimp to the list

297c510

kentcdodds reviewed Mar 19, 2016
View reviewed changes

ta2edchimp added 6 commits March 19, 2016 19:00

chore(dependencies): Add eslint as a peer dependency

5277e66

fix(lib): Restore original behavior when invoking without any argument

14a9de7

Restore the module's original behavior of interpreting the current package's main script as source of an eslint config, rather than taking the local package.json's eslintConfig property into account.

fix(tests): Alter tests to reflect the module's originally intended b…

ffb0f16

…ehavior

docs(usage): Clarify supported file types

212dc85

docs(usage): Add note about location restrictions of config files to …

e843a84

…be tested

docs(install): Add instruction on how to install locally as dev depen…

3fb35ab

…dency

sarbbottam reviewed Mar 19, 2016
View reviewed changes

ta2edchimp added 2 commits March 19, 2016 20:12

docs(usage): Clarify supported config format

7bb5d07

docs(contributors): Added ta2edchimp as contributor to doc

519b643

kentcdodds pushed a commit that referenced this pull request Mar 19, 2016

Merge pull request #4 from ta2edchimp/support-extends

6cedc7f

Support `extends`

kentcdodds merged commit 6cedc7f into kentcdodds:master Mar 19, 2016

jamestalmage mentioned this pull request Mar 21, 2016

Recipe for mocking the filesystem avajs/ava#665

Closed

Support extends #4

Support extends #4

Conversation

ta2edchimp commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Mar 16, 2016

ta2edchimp commented Mar 16, 2016

ta2edchimp commented Mar 16, 2016

kentcdodds commented Mar 16, 2016

kentcdodds commented Mar 16, 2016

ta2edchimp commented Mar 17, 2016

kentcdodds commented Mar 17, 2016

sarbbottam commented Mar 17, 2016

codecov-io commented Mar 19, 2016

Current coverage is 100.00%

ta2edchimp commented Mar 19, 2016

ta2edchimp commented Mar 19, 2016

ta2edchimp commented Mar 19, 2016

kentcdodds commented Mar 19, 2016

ta2edchimp commented Mar 19, 2016

kentcdodds commented Mar 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Mar 19, 2016

ta2edchimp commented Mar 19, 2016

ta2edchimp commented Mar 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Mar 19, 2016

kentcdodds commented Mar 19, 2016

Support `extends` #4

Support `extends` #4

Current coverage is `100.00%`