Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not accepting nested configurations #26

Closed
alexlafroscia opened this issue Mar 18, 2016 · 6 comments · Fixed by ember-cli/ember-ajax#78
Closed

Not accepting nested configurations #26

alexlafroscia opened this issue Mar 18, 2016 · 6 comments · Fixed by ember-cli/ember-ajax#78
Milestone

Comments

@alexlafroscia
Copy link
Collaborator

While developing ember-cli/ember-cli-eslint#41, I initially bumped the broccoli-lint-eslint version to 2.0.1, just to see if that changed anything. I noticed that any files within the tests/ directory of the Ember app I was testing against were no longer using the .eslintrc file under tests/.eslintrc to overwrite the configuration provided by the config file in the root of the app. So, certain files that were passing before were failing after updating this dependency, and passed again after reverting to the original version.

@alexlafroscia
Copy link
Collaborator Author

I've added ember-cli-eslint to one of my Ember apps (not an addon) and am seeing this same issue. broccoli-lint-eslint@2.0.1 was installed by default, and the tests/.eslintrc config is not merged with the one in the root of the directory. So, ember test fails on files in the tests/ directory but running eslint tests/ passes.

@BrianSipple
Copy link
Collaborator

I'm noticing the same thing -- will investigate further soon.

@rwjblue
Copy link
Member

rwjblue commented Mar 22, 2016

Issue is slightly but not directly related to https://github.com/jonathanKingston/broccoli-lint-eslint/blob/master/lib/index.js#L80 (which is modified by #25).

Basically, we are using executeOnText which uses the cached options (which basically includes only the root directory .eslintrc.js file). Using executeOnFile would properly generate each files specific config.

For this to work for us, we basically need to be able to call executeOnText but provide options (built via CLIEngine.prototype.getConfigForFile), but that isn't possible. We can work around this by doing the following (I think):

var config = this.cli.getConfigForFile(pathToFile);
this.cli.options = config;
this.cli.executeOnText(contents, pathToFile);

But it really seems odd that the ESLint side doesn't expose this without ^^ ugliness. Maybe we can ping them?

@nickiaconis - Also, RE: broccoli-persistent-filter, we would want to use getConfigForFile for the file specific cache key. It might be too expensive, but it would be a good place to start....

@BrianSipple
Copy link
Collaborator

I’ve been trying to reproduce this within the test suite for broccoli-lint-estlint itself, but oddly, the test helper seems to handle nested configs just fine.

I pushed up the branch that I created for starting to dig in to this if anyone else is curious, and I’d expect https://github.com/jonathanKingston/broccoli-lint-eslint/blob/fix-nested-configuration-errror/test/test.js#L22 to fail on the nested fixture/config that I created without yet implementing any changes to the way this.cli is configured and this.cli.executeOnText is called.

But it passes. And, when I log out the return value for this.cli.getConfigForFile(pathToFile) in every test, the nested fixture does, indeed, use the nested .eslintrc. Most likely, I’m not running things the same way that they’d be run in a consuming app that does ember test… I can’t quite pinpoint it yet, though.

@nickiaconis
Copy link
Contributor

@rwjblue When you include a pathToFile in a call to executeOnText, ESLint calls the equivalent of getConfigForFile under the hood.

@BrianSipple
Copy link
Collaborator

Should be fixed after #25

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 a pull request may close this issue.

4 participants