Add eslint for javascript code consistency

[gauravtiwari]

Think would be good idea to have some kinda of linter so that we have a consistent javascript code. I saw that @guilleiguaran made a commit for code consistency a while ago, but in this PR #110 we got some es6 stuff too so, it would be good idea to have a linter that takes care of this for us. Obviously, we can fine tune linter as we like.

The idea is to have a set of standard aesthetics that more or less works for everyone and perhaps everyone can follow when committing javascript code to this repo and then we can setup travis later to take care of this in PR's.

To auto fix linter complaints, node_modules/eslint/bin/eslint.js lib/install --fix

[guilleiguaran]

is possible to ignore the semicolon and trailing commas rules?

[p0wl]

Not sure about this. What do you want to express with it? The code in this repository is half node (webpack) and half browser (hello_*.js).

Maybe it's good to define both envs:

{
    "env": {
        "browser": true,
        "node": true,
    }
}

http://eslint.org/docs/user-guide/configuring.html#specifying-environments

[gauravtiwari]

Good idea. Fixed 🎉

[p0wl]

react, jsx-a11y and import are already included in airbnb: https://github.com/airbnb/javascript/tree/master/packages/eslint-config-airbnb#eslint-config-airbnb-1

[gauravtiwari]

oh yes that's right @p0wl. Use the eslint command it sort of added it by default. Fixed it now

[p0wl]

Thanks for this! Added some remarks. Just because I was unsure at first: This is meant to lint the code of the gem rails/webpacker itself, and not the code of the application that uses webpacker, right?

Regarding semicolons: If no semicolons is prefered, add semi: ['error', 'never'], to the rules in .eslintrc.js

[gauravtiwari]

@guilleiguaran Yes, it is. I will add the rules 👍

[p0wl]

can you use double quotes here as well? =)

[gauravtiwari]

Hi @p0wl Yes, no problem. Thanks for pointing out. Yes, the linter is to lint the javascript code used in this Gem - examples and configs

[p0wl]

Maybe add a link task to package.json:

"scripts": {
    "lint": "eslint lib/install"
},

that way you can run it via yarn lint (or yarn lint -- --fix)

[gauravtiwari]

Yeah done that. My thought was to add this when adding travis, but it makes sense to add now. @p0wl and @guilleiguaran I think this is good enough now. Please review and merge!

[p0wl]

LGTM

[dleavitt]

❤️ Bump! This should get merged soon as it touches a ton of code.❤️

[guilleiguaran]

why this isn't a class anymore?

[dleavitt]

Functional components have a lot of cachet right now. I think it's a little too fancy for a bare-bones example but we can always fix it later.

[gauravtiwari]

@guilleiguaran Because we can do away with functional component. Perhaps, we can make a class example too, but I guess the intention of this gem to not teach react but rather setup an environment that works with react.

[justin808]

@guilleiguaran the linter will complain if you use a class when a simple function will suffice.

[justin808]

@gauravtiwari I'd recommend that you stick with the airbnb standard as-is and not ignore any rules.

@guilleiguaran commented 3 days ago
is possible to ignore the semicolon and trailing commas rules?

If we ignore the rule, then we'll inconsistencies.

[gauravtiwari]

@justin808 Yes, my initial intentions were to keep airbnb rules as it is, as it's fairly standard, JS community wide, however it felt later that since folks here seems to be persistent about not having the semicolons and commas therefore, perhaps we may just ignore it this time. There is not a lot of JS code here, only some configs so, probably won't cause a lot of issues. The linter will throw errors if someone adds semicolons or commas. Perhaps, we can automate this with travis going forward.

There is also some discussion (#124) on moving out third party integrations out of this gem so, this gem will only be responsible for setting up a webpack powered environment in any Rails app.

[gauravtiwari]

@guilleiguaran if you could clear this PR soon, would be great. There are a couple of things in pipeline that are currently blocked due to this one.

[justin808]

@gauravtiwari Run with the herd. The configs used by Airbnb are used on just about 100% of the various real-world customer projects that I see. Given the number of downloads, I would be more hesitant to make exceptions, than to just accept as-is for now. We can always change this later.

[justin808]

@gauravtiwari If you're going to deviate from the very popular airbnb standard, then please document your reasoning on why.

For some of the rules, like no-console, that should be overridden where used. Otherwise, you'll have developers including random debugging statements in PRs that pass linting.

[gauravtiwari]

@justin808 I personally have no problem with just using airbnb standard, it's just that a couple of active contributors and members here pointed out to not have semi-colons and commas due to aesthetics, so it's been removed for now. We can always add it later.

Sure, I see your point for no-console. I will fix it 👍

[gauravtiwari]

@justin808 not necessarily, and I completely see your point, however it also seems to me that in the context of this Gem, this won't cause a lot of problem because it's mostly Ruby and sprinkles of javascript, which are basically Webpack config files. There is also discussion on moving out all third party integrations out of this Gem so, that means even less javascript code.

Obviously, not everyone's aesthetics are same, considering folks who have written mostly ruby and coffeescript in the past, to them semi-colons would look rather hideous. So, just for that I thought we keep it as-is for now, and see how it goes. As you said, we can always change it, if any real problem arises.

[dleavitt]

IMO, these are all valid points but we might want to just get this merged (as it's going to get stale fast) and then debate styling later. Long-term solution here is probably match whatever JS styleguide basecamp uses.

[dhh]

I'm fine with using lint but we don't have to slavishly following airbnb style or whatever. Fine for us to divert in consistent ways. For me, semicolons is absolutely a no go. Every one of them a thorn in the eye. Not crazy about removing equal-sign alignment on multiple definitions either. And the localMap one doesn't make sense to me either.

…

On Feb 27, 2017, at 17:57, Daniel Leavitt ***@***.***> wrote: IMO, these are all valid points but we might want to just get this merged (as it's going to get stale fast) and then debate styling later. Long-term solution here is probably match whatever JS styleguide basecamp uses. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[dleavitt]

@dhh I agree that the airbnb style is a little bloated and over-prescriptive. Do you guys use eslint anywhere in Rails or Basecamp? If not maybe an elegant, wabi eslint config should be created for use across all Rails JS?

But maybe not this second. This second I think we should just merge (or close) this branch and tweak later. It's blocking things.

[gauravtiwari]

@dhh Agree, it all depends on what works here for now. LocalMap is basically one of those immutable conventions in JS, where the original param isn't modified but a copy is created and used for new assignments.

Is everything else makes sense? Feel free to adjust and merge this please :)

[dhh]

Happy to work on a lint that reflects our Ruby-tinted aesthetics. But yes let's keep moving. As long as we are not adding semicolons we can keep moving.

…

On Feb 27, 2017, at 19:07, Daniel Leavitt ***@***.***> wrote: @dhh I agree that the airbnb style is a little bloated and over-prescriptive. Do you guys use eslint anywhere in Rails or Basecamp? If not maybe an elegant, wabi eslint config should be created for use across all Rails JS? But maybe not this second. This second I think we should just merge this branch and tweak later. It's blocking things. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[gauravtiwari]

Great, think this is good enough then. Please merge 😄

[justin808]

As a reference in regards to the semicolons, the main problems with ASI are some subtle cases that cause no syntax errors and defective code. This is much less obvious than the rules regarding line endings for Ruby and CoffeeScript

Here is a good article on the topic: Understanding Automatic Semicolon Insertion in JavaScript

On the flip side, I've never experimented to see if the most recent versions of ESLint will flag the common error cases. It may, and, if that were the case, then the older arguments against semicolons may not be as relevant.