Accept non-string input trees

[nickiaconis]

This allows the input tree to be a real tree or a string.

[nickiaconis]

What I've proposed here is a rather simple solution, and I'd like to discuss it. I'm sure there are cases I've overlooked. I'm also pretty certain it can't handle broccoli-merge-trees as it's currently written.

[nickiaconis]

Ping.

[rwjblue]

Hmm, I generally think that we need to not rely on knowing the root.

[nickiaconis]

I don't see how we do that and support ESLint's configuration inheritance.

[nickiaconis]

I've updated this to rebase with master, but it now depends on #29.

[BrianSipple]

This is interesting because it appears like it could be a solution for #26. (See my below side note).

However, I’m having some trouble running ember test in an ember app with this setup alongside ember-cli-eslint.

When resolveInputDirectory(inputTree) is called recursively on a non-string, it fails after the second call when attempting to access inputTree._inputNodes[0]. This is because it's passed an instance of WatchedDir, which looks like this:

{
 _directoryPath: 'app',
  _watched: true,
  _name: 'WatchedDir',
  _annotation: undefined,
  _instantiationStack: [….long stack trace….] '
}

This appears to stem from what ember-cli-eslint is passing as an inputTree during its lintTree hook, but I haven't been able to trace that process further yet.

_Side note_:
As a super-dirty experiment, I added logic to resolveInputDirectory(inputTree) in my local branch to have it return the result of path.join(process.cwd(), inputTree._directoryPath) if it couldn't find inputTree._inputNodes. This worked and it also picked up the .eslintrc config that I had nested inside of my tests/ directory -- which is exactly what we're looking for in #26.

That said, it doesn't feel it should be the responsibility of anything here to have to know about handling something which doesn't appear to extend fully from Broccoli.Plugin. That feels like more of a responsibility for a consumer of the module (such as ember-cli-eslint), but I could be wrong.

[nickiaconis]

Hey @BrianSipple! Lots of good info there. It led to me the Broccoli Node API, from which I learned about the node.__broccoliGetInfo__ method. That method really helps with tracking down the source directory, including in the case you mentioned.

I've updated the commit based on this new information. I'm beginning to feel a lot more comfortable with this approach. It still doesn't support merged nodes, but I don't think it's a problem to ask people to do their linting before merging their nodes, assuming it fails with a useful error message instead of silently behaving in an unexpected manner.

I also think this assumption is in line with the way ember-cli works. I've npm link'd these changes into ember-cli-eslint and a new app in order to test it. It'll require a small change to ember-cli-eslint to skip linting the templates tree, but I don't think that's a problem either.

Coincidentally, I also needed to rewrite the runEslint helper, and I'm much happier with it now. No more stubbing console.log. 😛

[BrianSipple]

Awesome find! Checking the nodeInfo avoids the previous error I was mentioning — and changes my mind about not being able to handle it here: It seems like that’s exactly what the Broccoli Node API is trying to enable, actually 😉

There is one issue, however, that the current implementation here seems to introduce. In a local Ember app where I have this npm linked, the error reporter ignores the nested directory structure when logging out the filename of the errors — for example, printing [pathToProject]/tests/component-test.js for every file in tests/ -- even though the actual file would be something like tests/components/svg-icon/component-test.js. (As a result, I have errors coming from a long list of different files all named tests/component-test.js 😀).

My solution for this was to replace

  const configPath = path.join(this.eslintrc, path.basename(relativePath));

with

  const configPath = path.join(this.eslintrc, relativePath);

This allows us to call executeOnText using the full relative file name, and nested configs are still detected/used because, thanks to having it be the result of resolveInputDirectory(), this.eslintrc has already been set to the properly nested root by the time we get here.

[nickiaconis]

Oh, haha, yeah, you're totally right. Will fix when I'm at a computer.

[BrianSipple]

@nickiaconis I think the CI errors here will be fixed by pulling in the latest master now that #29 has been merged.

Barring that, this looks good to go, as does #21 and #30 -- we just need to determine the best merge order. I'm sensing #25, #21, then, #30 -- which also seems like it should warrant a patch version bump (which ember-cli-eslint, for one, could use for it's own milestones 😄 ).

[nickiaconis]

@BrianSipple Thanks for the ping. I've rebased with master, and tests are passing.

[BrianSipple]

If there are no objections on anything, I'd like to start merging #25, #21, and #30, and then bump to 2.2.0 for ember-cli-eslint

/cc @nickiaconis @rwjblue @jonathanKingston