Skip to content
This repository has been archived by the owner on Nov 23, 2024. It is now read-only.

Consider removing babel-eslint from coding-standard #81

Closed
kellyselden opened this issue Jun 22, 2016 · 15 comments
Closed

Consider removing babel-eslint from coding-standard #81

kellyselden opened this issue Jun 22, 2016 · 15 comments
Labels

Comments

@kellyselden
Copy link
Member

I would like to extend from coding-standard/ember-application.js, but I'm not using the babel-eslint parser. The default espree parser works just fine with default ember-cli apps. I tried overriding as parser: 'espree', but I'm still getting Error: Cannot find module 'babel-eslint' in the console because I have eslint installed in my package.json but not babel-eslint.

@alexlafroscia
Copy link
Collaborator

This is something that there's been a lot of back-and-forth on. My perspective has been that babel-eslint makes more sense as a default so that there's only one parser for the code to satisfy, instead of Babel and Espree. Is there a reason that babel-eslint doesn't work for you? Or is it just a preference thing?

@kellyselden
Copy link
Member Author

I have my own local install of eslint in my package.json for various reasons, better command line access in npm scripts, better version control, better atom linting resolution. The only downside is now I can't use the coding-standard/ember-application.js because it requires babel-eslint, which my local eslint can't resolve. I always prefer to avoid as many dependencies as I can to avoid npm dependency hell, which is why I suggest removing the dependency on the babel-eslint parser.

@BrianSipple
Copy link
Collaborator

I’m starting to come around to the side of #TeamKeepItOut. One more reason — in addition to what @kellyselden has already pointed out — is that the default parser has been improving drastically and now fully supports all standardized ES6/ES7 features — or in other words, the code you’ll be running when you get started with an new Ember application.

Furthermore, the more I use ESLint in general, the more the act of adding a custom parser does, indeed, feel like a significant "extra step” — and potentially just the first among many, which can include custom plugins and other unique preferences. With that in mind, I think we might be better off now, and in the long run, by allowing users to have full control over this process.

@alexlafroscia
Copy link
Collaborator

alexlafroscia commented Jul 13, 2016

Hey, if Espree supports all the ES6/7 stuff too, I'm in favor of leaving the default configuration as well.

@BrianSipple BrianSipple added the RFC label Aug 3, 2016
@Turbo87
Copy link
Member

Turbo87 commented Aug 11, 2016

@BrianSipple @alexlafroscia @kellyselden I guess you know my opinion on this by now. How can we decide this?

@kellyselden
Copy link
Member Author

I think we are all on the side of take it out now.

@kellyselden
Copy link
Member Author

It would probably need a major version bump though, because code may fail to validate once we change it.

@rwjblue
Copy link
Member

rwjblue commented Aug 11, 2016

Agreed with both of @kellyselden's points above. We should remove, but it will require a major bump.

@Turbo87
Copy link
Member

Turbo87 commented Aug 11, 2016

if we take the parser out of the base config files then what remains? should we remove them as well and move the remaining pieces in the blueprints instead?

@alexlafroscia
Copy link
Collaborator

@Turbo87 I think that sounds like a really good idea

@kellyselden
Copy link
Member Author

https://github.com/ember-cli/ember-cli-eslint/blob/master/config/eslint.js

I think there is enough in there to keep the extending the way it is. Then whenever greenkeeper updates my ember-cli-eslint install, I have less to worry about. The default recommended rules may change, but I don't need to run the generator again if all the rules are stored somewhere else. Hope that makes sense.

@alexlafroscia
Copy link
Collaborator

alexlafroscia commented Aug 14, 2016

That file is what this repo uses for it's own ESLint config. I think the file we're recommending we remove, and push into the blueprints, is this one:

https://github.com/ember-cli/ember-cli-eslint/blob/master/coding-standard/ember-application.js

I do see what we're saying about wanting to rely on a external source for the rules for update purposes, but I'm not sure we actually want to provide those rules in ember-cli-eslint. I kinda feel like this repo should just be the linter rules, and if we want to come up with a recommended default setting for Ember apps, that should be in a separate ESLint config/plugin repo instead.

@Turbo87
Copy link
Member

Turbo87 commented Aug 15, 2016

I think there is enough in there to keep the extending the way it is.

you are correct that there are still useful things in there, but those would fit much better into a "shareable config". shareable configs are the default way to specify a set of rule configurations in ESLint and I don't see a reason why we would need to invent another solution for this.

after writing those sentences I noticed that @alexlafroscia summarized it almost the same way and also he's write that the relevant file that I was talking about is actually ember-application.js, which is not yet filled with any custom rule configurations.

@alexlafroscia do you have time for a PR that removes those files?

@alexlafroscia
Copy link
Collaborator

Yup, definitely. I'll get that together today.

@kellyselden
Copy link
Member Author

Yea I was mistaken on the file, it's less complex than I thought.

alexlafroscia added a commit to alexlafroscia/ember-cli-eslint that referenced this issue Aug 15, 2016
*Note:* This is definitely a breaking change that will require a major
version bump and a note in the `CHANGELOG` about the fact that the
`coding-standard` files no longer exist (in case someone is upgrading
and currently extending from them).

Closes ember-cli#81
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants