Rewrite autobind calls as class properties

[smashwilson]

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

Rewrite bound functions throughout the package from using the autobind() helper function:

import {autobind} from '../helpers';

class Something {
  constructor() {
    autobind(this, 'someMethod');
  }

  someMethod() {
    //
  }
}

To using class properties:

class Something {
  someMethod = () => {
    //
  }
}

I've also made a small number of related changes:

• Constructors which were now empty without the autobind() call have been removed.
• Bound methods which were single-line have been changed to single-line arrow functions where they fit within the maximum line length.
• Several methods that called React's setState() were modified to return a Promise that resolves when the component's re-rendering is complete.

Alternate Designs

The autobind helper was introduced in #1430 to replace the @autobind decorator. Decorator support was the only reason we had a dependency on babel-eslint, which was causing conflicts with a Relay upgrade.

Rather than use class property syntax, we could:

Keep the autobind helper

The way we have it now is kind of a hack, but it does the job.

Downsides: non-standard and potentially surprising for contributors. It requires us to keep the list of method strings up-to-date with the methods defined on the class and fails hard if they drift. It also makes it non-obvious which methods are bound and which are not at the method declaration sites.

Re-introduce decorator support

We'd need to find a way to do this without breaking babel, istanbul, and Relay.

Downsides: I couldn't figure this out the last time that it came up, so I'm hesitant to give it another shot, especially because (as I understand it) the future of decorators in JavaScript proper is uncertain.

Hand-bind functions when used

We could use .bind() to bind methods when they're used in a context that requires them to be bound:

class SomeComponent extends React.Component {
  render() {
    return (
      <OtherComponent someMethod={this.someMethod.bind(this)} />
    );
  }

  someMethod() {
    //
  }
}

Downsides: it's pretty inefficient; this will create a new Function object on every render, which will be referentially unequal, which can in turn prevent OtherComponent from being optimized to skip unnecessary renders by detecting when its props are unchanged. Furthermore, if more than one callsite needs a bound function, different Functions will be created for each of those. Also, it isn't obvious at the method declaration site whether the method is bound or not.

Using an arrow function directly in render() has similar drawbacks.

Hand-bind functions within constructors

We could use .bind() to manually bind functions within class constructors:

class Something {
  constructor() {
    this.someMethod = this.someMethod.bind(this);
  }

  someMethod() {
    //
  }
}

Downsides: it's very verbose, it requires constructors on many components that otherwise wouldn't need them, and it makes it non-obvious which method are bound and which are not at the method declaration site.

Benefits

The biggest immediate benefit will be consistency. We'll have a single pattern for bound methods throughout our codebase, which should reduce surprise as we move around, etc.

This pattern should also be less surprising to contributors, as (unscientifically) React developers are moving toward class properties:

Possible Drawbacks

It's possible that I could accidentally introduce a regression here. Our tests should catch it though 🤞

Applicable Issues

I could swear I saw @kuychaco file one about this but I cannot for the life of me find it now.

Metrics

n/a

Tests

n/a

Documentation

n/a

Release Notes

n/a

User Experience Research (Optional)

n/a

[codecov]

Codecov Report

Merging #1850 into master will decrease coverage by <.01%.
The diff coverage is 78.22%.

@@            Coverage Diff            @@
##           master   #1850      +/-   ##
=========================================
- Coverage    90.9%   90.9%   -0.01%     
=========================================
  Files         195     195              
  Lines       10727   10584     -143     
  Branches     1570    1569       -1     
=========================================
- Hits         9751    9621     -130     
+ Misses        976     963      -13

Impacted Files	Coverage Δ
lib/helpers.js	89.85% <ø> (+0.23%)	⬆️
lib/controllers/conflict-controller.js	41.02% <0%> (-1.48%)	⬇️
lib/views/github-login-view.js	40.9% <0%> (+2.44%)	⬆️
lib/views/pr-commits-view.js	77.77% <100%> (+11.11%)	⬆️
lib/controllers/github-tab-controller.js	100% <100%> (ø)	⬆️
lib/containers/changed-file-container.js	100% <100%> (ø)	⬆️
lib/atom/pane-item.js	93.75% <100%> (-0.07%)	⬇️
lib/items/changed-file-item.js	100% <100%> (ø)	⬆️
lib/views/issueish-list-view.js	100% <100%> (ø)	⬆️
lib/views/accordion.js	83.33% <100%> (-0.54%)	⬇️
... and 50 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2376e9...33b0a0c. Read the comment docs.