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

eslint shareable configs / split #366

Closed
orlin opened this issue Jun 18, 2015 · 25 comments
Closed

eslint shareable configs / split #366

orlin opened this issue Jun 18, 2015 · 25 comments

Comments

@orlin
Copy link

orlin commented Jun 18, 2015

Because the react plugin / jsx rules aren't optional with this style-guide, I found out there is an official way for sharing eslint configs. The "standard" style, with standard-react, makes a good example of splitting eslint rules. Not all js is front-end, and using react...

@justjake
Copy link
Collaborator

Ok, sounds good. I will try to move React styleguide options from eslint-config-airbnb to eslint-config-airbnb-react next week. I'm also planning to do an audit of the .eslintrc to make sure it conforms with both the letter and the spirit of the styleguide.

@goatslacker @hshoff should we have 3 separate ESlintrcs? eslint-config-airbnb, eslint-config-airbnb-es6, eslint-config-airbnb-react? I would prefer not to split out the ES6 rules since those are considered part of the core style guide, but I'm up for discussion on the matter. I would prefer to only maintain two, eslint-config-airbnb and eslint-config-airbnb-react.

@goatslacker
Copy link
Collaborator

+1 on just two. We can have a third es5 only if people really want that but since ES2015 is a final draft and its actual JS now I vote we move forward with no distinction between the two.

@ljharb
Copy link
Collaborator

ljharb commented Jun 18, 2015

The spec being finalized is not the same as being free to use ES6 sans transpilation - ES5 was finalized in 2009 and it wasn't safe to use getters/setters for many many years. I'd prefer have an ES5 one as well for those who don't wish to use babel, and wish to support browsers.

@baer
Copy link

baer commented Jul 5, 2015

Hey guys, we (walmartlabs) have been grappling with updating our ESLint config as well. Especially making sure that it works in multiple environments (ES5, ES6, React, Node, Browser, Test etc.) and we decided to take the same approach and split our config out into an ES5 base + extensions. It's been working out well so far.

We also had issues with protecting CI from new rules that get turned on when we update ESLint and maintaining the giant YAML files so we created eslint-config-defaults. It starts with a reset where everything is off (this will be default in the upcoming ESLint 1.x anyway) then builds up config with composable rule sets using ESLint's sharable config.

So far we have WalmartLabs configs, the default set from ESLint and a (decomposed) copy of your eslintrc file. I'd love to work together if you are interested in trying to follow a similar path. Ultimately we'd like to try to provide more premade, robust and easily consumable configs available to the community. It would be great to have something akin to the JSCS Presets at some point.

Either way, I thought I'd point the conversation towards an example that has been working well :)

Thanks for putting this guide together by the way, I've been using chunks of it in personal projects for a few years now. Awesome work!

@hshoff
Copy link
Member

hshoff commented Jul 6, 2015

As long as we're supporting the ES5 guide we should provide a working eslint config as well.

Similiar to how @baer is doing it, I like how @segmentio is handling this: https://github.com/segmentio/eslint-config

@baer
Copy link

baer commented Jul 7, 2015

That would be rad. One thing to note is that ESLint merges the user defined config with their default config whether you like it or not so the config as read by eslint might not be an exact match to what you give it unless you define all of the properties.

As a part of my project I took the config you have here and did that merge before I divided it up so if you want to break stuff apart you could consider copying/forking/whatever-ing what I have since it may be duplicate work in some areas.

@thefotios
Copy link

Has there been any update on this front? We (Shutterstock) are looking to adopt this styleguide, but have a mix of ES6/5 and would like to use the same styleguide. If nobody has taken a try at it, I can give it a go and submit a PR.

@nkbt
Copy link

nkbt commented Jul 29, 2015

@thefotios have you tried eslint-config-airbnb?

npm install -SD eslint-config-airbnb

In .eslintrc

{
  "extends": "eslint-config-airbnb"
}

For different code in different subfolders just create separate .eslintrc files with different rules.

@thefotios
Copy link

@nkbt Yea, it definitely works just fine, but we're running into issues with some rules not being applicable to ES5, like no-var. It might make sense to have a "base" ruleset that just has style rules, then an ES5 ruleset that extends that, and an ES6 ruleset that extends either of those, and so on for browser, react, whatever.

Not entirely sure how the extends works under the hood with eslint, but if it is just requiring the extends value, then this would work.

Module structure:

.
├── index.js
└── versions
    ├── browser
    │   └── index.js
    ├── es6
    │   └── index.js
    └── react
        └── index.js

.eslintrc

{
  "extends": "eslint-config-airbnb/versions/browser"
}

Theoretical eslint logic

var rcFile = require(".eslintrc");
var baseRules = require(eslintrc.extends);
// require "package/foo" overrides the module's "main" and looks for an "index.js" in that dir

@nkbt
Copy link

nkbt commented Jul 29, 2015

@thefotios I see what you mean... You can create additional config overriding some rules for es5 (will be quite short, and then use it for your legacy folder. So you just keep airbnb as the core one, and override rules for es5-one.

In my projects I have 3 configs: main for actual js code, one for tests, one for nodejs/configs. You can check it for example in https://github.com/nkbt/react-component-template (though I don't use airbnb because I want much higher strictness and include all supported eslint rules).

@thefotios
Copy link

@nkbt Thanks, that is the approach we're taking for now.

In case anybody is still following this, doing an extends: package/foo works as I theorized in a previous post. Even better those can extend the base eslint config for the package as exposed via the package.json's main directive.

{
  "main": "es6/index.js"
}
// airbnb/es6/index.js
module.exports = {
  rules: {
     // babel inserts "use strict"; for us
    strict: [2, "never"],
  }
}

Then we could make the es5 version, es5/index.js. This will work because the extends is evaluated at run time, so it's not a circular reference to its own package.

// airbnb/es5/index.js
module.exports = {
  extends: "airbnb",
  rules: {
    // We always want strict and we don't have babel to help us :(
    strict: [2, "global"] 
  }
}

@nkbt
Copy link

nkbt commented Aug 3, 2015 via email

@brettstack
Copy link

How about creating the separate configs as suggested (es5, es6, react) and extend using multiple extends http://eslint.org/docs/user-guide/configuring.html#extending-configuration-files. So put all the core rules in es5 (I assume most people are stuck on this), and then have the es6 and react configs override/add the necessary rules. So the consumer .eslintrc files will look like:

For es5:

extends:
  - eslint-config-airbnb

For es6:

extends:
  - eslint-config-airbnb
  - eslint-config-airbnb-es6

For react:

extends:
  - eslint-config-airbnb
  - eslint-config-airbnb-react

For es6+react:

extends:
  - eslint-config-airbnb
  - eslint-config-airbnb-es6
  - eslint-config-airbnb-react

@ljharb
Copy link
Collaborator

ljharb commented Aug 9, 2015

That is an excellent idea because it would also illustrate what additions and changes there are between the spec versions, and react.

I'm pretty sure that es6 itself could extend the es5 one, so that consumers wouldn't need to stack them - ie, each one could handle its own dependencies.

@baer
Copy link

baer commented Aug 9, 2015

I've already done the bulk of that for you if you'd like to use it. I just updated to ESLint 1.0.0 + AirBnB 0.0.7 this week so you should be able to just take it as is.
https://github.com/walmartlabs/eslint-config-defaults/blob/master/configurations/airbnb.js

You should check out how WalmartLabs does config for an example of how configs can be composed
https://github.com/walmartlabs/eslint-config-defaults/tree/master/configurations/walmart

@justjake
Copy link
Collaborator

@orlin the official package eslint-config-airbnb now has a react-free version (as of v0.0.8)!
simpy npm install --save-dev eslint-config-airbnb and add { "extends": "airbnb/base" } to your .eslintrc

see here: https://www.npmjs.com/package/eslint-config-airbnb

@baer eslint-config-defaults looks great! I think I'll end up pulling your work into this repo sooner or later since the organization is much better than mine :)

@justjake
Copy link
Collaborator

I'm going to leave this issue open until i merge all the walmart labs stuff

@baer
Copy link

baer commented Aug 21, 2015

@justjake - Right now I'm manually porting all of the rules and I should be current up to the NPM release 0.0.7. If you're thinking of pulling that stuff in - I'd love to be able to just take this project as a dependency of eslint-config-defaults. Do you think you would keep the folder structure such that I could expose config in the same way?

@justjake
Copy link
Collaborator

@baer yeah, that sounds great. ping me when you've pulled in the React change from 0.0.8 and I'll switch over

@baer
Copy link

baer commented Aug 22, 2015

Sure, I can probably have that done pretty quickly. Looks like somebody forgot to push the 0.0.8 tag though.

@tomek-he-him
Copy link
Contributor

@justjake there have already been a couple of useful changes to eslint-config-airbnb since 0.0.8. Could you kindly roll out a new release?

@justjake
Copy link
Collaborator

@tomekwi i rolled out 0.0.9 a few days ago

@justjake
Copy link
Collaborator

ok, @taion has done the hard work of pullin in @baer's eslint-config-defaults in #526. Does this look like a good solution to everyone here for the basis of our eslint config going forward?

@justjake
Copy link
Collaborator

Merged the segmented config.

@tomek-he-him
Copy link
Contributor

👍 Thanks, @justjake!

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

No branches or pull requests

10 participants