add eslint to the project

[knowbody]

This is going to solve #57

I thought it would be good to add eslint to the project at this stage. I have borrowed the eslint rules from react repo. I have fixed most of the linting errors, however in those I was not sure how you guys want to deal with refactoring I left there and added /* eslint ... */ on top of the file to just generate the warning for now.

The most common issues are:

• no-shadow
• no-unused-expressions
• and also sometimes warnings and invariants are missing a error message

Please let me know how would you like to fix the above and I do those changes.

[wincent]

This is awesome. I think we'll want to do something better than copy-pasta the React files as-is though.

On the subject of all the shadow warnings, it's such a common idiom in the codebase that I wonder whether we should just disable that lint (in some cases the readability actually suffers due to the awkward variable names).

[josephsavona]

@wincent Agreed about disabling shadow warnings.

[knowbody]

@wincent @josephsavona cool I'll disable it and fix the issues. I copied react's one just to start this off, thought that you might have some kind of guidelines at Facebook. Any other suggestions?

what do you think about no-unused-expressions?
at the moment there is a lot of places where we do something if the expression is true, e.g.:
in src/container/RelayContainer.js (256):

this.pending && this.pending.request.abort();

with no-unused-expressions it would need to look like:

if (this.pending) {
  this.pending.request.abort();
}

not sure which one do you prefer?

---

also not sure why the tests are failing on travis, seems like some issue while installing from npm

[josephsavona]

Thinking about the implications of this more: all of the code passes our internal lint checks, and it will complicate syncing if the ESLint config here is more restrictive than our internal linting. Can you relax the rules just enough to not require any changes to files in src/? E.g., allow for missing trailing commas, disable no-unused-expressions and no-shadow.

Thanks again for collaborating on this!

[zpao]

Next on my list for fbjs is to get our lint config in a good place to be shared and reused. For React we opted to be more restrictive than the rest of FB because we could be (we sync in, not out so it doesn't matter).

FWIW, we actually might not be passing internally, we should force a full check :)

no-unused-expressions is off internally (there were too many uses, but is one of those cases where we opted to be stricter in React) so we shouldn't change those here. Not sure of no-shadow off the top of my head.

[knowbody]

@josephsavona ok, I'll do that and update the PR. @zpao I think for now let's just add the basic ESlint rules and make sure they pass, and then we can add the ones which we see there is a need for?

[knowbody]

@josephsavona updated the code, let me know what you think.

travis is still failing while trying to run npm install

Edit:
was just looking at your style guide and you have included trailing commas, so in that case the style guide should be updated or leave the comma-dangle in ESlint rules

[zpao]

This should just be eslint . (npm adds node_modules/.bin to the PATH)

[josephsavona]

@knowbody I'm sorry if my previous comment wasn't clear. The addition of ESLint here should require no changes whatsoever to the src/ directory. This will ensure that future syncs between OSS and our internal repository can proceed smoothly. Let's disable the comma-dangle rule (also it appears some variables were renamed - leftover fixes from no-shadow, maybe?

Regarding style: we do prefer trailing commas, but this is the one preference for which we don't also have an internal lint rule (and so there are some inconsistencies in the existing code).

[knowbody]

@zpao thanks for the hints 👍

@josephsavona check it now please. I think I fixed all you asked for

[knowbody]

@josephsavona @wincent is anything blocking this to be merged?

[josephsavona]

We cannot accept this PR if there are changes to the src/ directory - rules should lenient enough to allow the code to pass as-is.

[josephsavona]

@knowbody See comments above. Again, we sincerely appreciate all the effort you've put into this, but to simplify syncing code for now the configuration should consider the code valid as-is, and there should be no changes to the src/ directory.

[knowbody]

@josephsavona sure, sorry fixing all that now

[knowbody]

@josephsavona should be good to go now

[zpao]

You'll be able to drop a lot of this, apart from some finer tuning, once we pull in facebook/fbjs#49 (might end up being better to end up abandoning this PR entirely and starting clean).

[knowbody]

@zpao just seen the merge you've done with react. agree, let's close and I start the new PR with the rules from fbjs repo. How does it sound? @josephsavona

[josephsavona]

closing and moving discussion to #202