Airbnb JavaScript styleguide

[janpanschab]

Consider implementing Airbnb JS styleguide. It’s great - https://github.com/airbnb/javascript. https://github.com/airbnb/javascript/tree/master/packages/eslint-config-airbnb

[vbulant]

+1

[kettanaito]

I suggest to create our own eslint-config-actum module and store all the rules there.

Why:

1. To clean package.json When talking about ESLint rules, they may be many. Putting them all into package.json will make it less readable for developers. Putting them in .eslintrc is good, but this way you need to drag this file wherever development stack goes. When the same may be done like this: { "eslintConfig": { "extends": "actum" } }
2. Consistent configuration It is possible to load not only the rules, but any other ESLint options from the module. The latter include environments, globals, ecmaScript parser versions, etc. (any option according to ESLint specification). Included once in the module, no need to configure them in the development stack. Plus: if you need to use a certain options (rules, anything) for a specific projects, you may just include them in package.json, and they will overwrite the options imported from eslint-config-actum just for this very project.
3. Easy way to declare different rules for development and production As stated by @janpanschab, it is likely that production linting will be much more strict. To not duplicated the rules, declare them manually in gulp task, I propose a dynamic solution. You may see it in action in the prototype of eslint-config-actum. Take a look how dev.js extends production config with making some rules less strict for us, developers.
4. Better maintenance
   • Module is easy to update via GitHub/npm, and easy to update in the project
   • Module loads its dependencies on its own. This way if it is required in the future to extend it, we don't need to go to each project's package.json and, for example, removing AirBnB styleguide (in case we want to use something different).

How: Taking eslint-config-airbnb as an example, I have written a simple npm module and put it on GitHub. Being tested within our latest gulp-dev-stack, it works fine, and it is easy to say Gulp tasks to use development rules when required. It still needs polishing, of course.

What's next: Regardless of the outcome of my suggestion, it would still be tremendously helpful for us to configure our production and development rules. I believe this is up to you, guys - @janpanschab @vbulant.

Let me know what you think about this suggestion.

[janpanschab]

Nice summary. I will check it ASAP. Thx

[kettanaito]

Update: I would really recommend to implement this logic toward the linting:

• While on development, linting gives you warnings (not blocking the process) about such things as console instances, multiple lines, etc. So you are not stopped by this, but you still know about it
• When linting the code for production, aforementioned warnings become errors, which will stop the code from building unless they are fixed.

Basically, this is from my experience, when there are some multiple lines occasionally left over, and right now linting does not warn you about it, unless you run build (which is time consuming sometimes). So I would recommend to always lint the rules, but make a different level warnings on development and production.

[vbulant]

Agreed. We have something similar currently in gulp/tasks/lint.js. It takes production rules from package.json and then softens them in case you’re in DEV environment.

[janpanschab]

@kettanaito Could you test the linter on some our current project code? ETL or Zindulka and extract the problematic rules? Then we can establish a commission and vote for rules settings :-)

[kettanaito]

@janpanschab okay. I will keep this message updated with the differentiable rules for development/production.

Rules list

These rules should be strict (prevent from build) on production and warnings on development. If we set proper strictness level on these rules, I think it makes sense to set failOnError (or its equivalent) to true, because errors should prevent from build.

• no-multiple-empty-lines May happen by accident, and should not prevent from development, but rather be present in the terminal as a warning to look out for.
• no-console Essential for debugging, and often left out.
• no-alert Essential for debugging, and often left out.
• quotes To not bother when, for example, trying some external solution, which does not share our lint rules. These occasions do happen, and the point during them is to find out the best solution, without spending time on correcting somebody else's coding style. If solution works - you see the warning in the terminal, and then you may clean it up.

[vbulant]

Overrides from este:

// Soft some rules.
"new-cap": [2, {"capIsNew": false, "newIsCap": true}], // For Record() etc.
"no-class-assign": 0, // Class assign is used for higher order components.
"no-nested-ternary": 0, // It's nice for JSX.
"no-param-reassign": 0, // We love param reassignment. Naming is hard.
"no-shadow": 0, // Shadowing is a nice language feature. Naming is hard.
"no-underscore-dangle": 0, // It's classic pattern to denote private props.
"react/prefer-stateless-function": 0, // We are not there yet.
"import/imports-first": 0, // Este sorts by atom/sort-lines natural order.
"react/jsx-filename-extension": 0, // No, JSX belongs to .js files
"jsx-a11y/html-has-lang": 0, // Can't recognize the Helmet.
"no-confusing-arrow": 0, // This rule is super confusing.
// Extension of rules.
"import/extensions": 2, // Ensure consistent use of file extension.

[kettanaito]

@vbulant Thank you. Do you think these are all production rules?

[vbulant]

@kettanaito Yes, I do. We use them in production on FirstScreen/Remy.