jhusain / eslint-plugin-immutable

ESLint plugin to disable all mutation in JavaScript.
Apache License 2.0
912 stars 17 forks source link

Option to allow mutation of this inside class constructors #16

Open axefrog opened 8 years ago

axefrog commented 8 years ago

I'm using immutable classes as clean, transportable and easy-to-debug-and-maintain wrappers around Immutable.js state objects in order to clearly define the shape of the underlying data. Such a class is constructed by passing it an instance of the state object in question. Operations which makes changes to state instead return a new instance of the class, constructed with the new state object created as a result of the operation.

Obviously this means I use:

{
  "immutable/no-let": 2,
  "immutable/no-this": 0,
  "immutable/no-mutation": 2
}

In the constructor, initialising the one-and-only class field this.state violates the "no-mutation" rule.

Because a constructor is an initialiser of sorts, in the same way that const a = b is initialising the value of a, if I allow use of this, then no-mutation should not apply in a class constructor.

I understand that this cannot be safely used for constructors defined via an ES3/5 function, given that such functions cannot be guaranteed to be used as a constructor, but for ES6 classes, the constructor is guaranteed to only be used during construction, and it can thus be inferred that we're initialising a new value (the class instance) and not mutating an existing value.

The workaround is adding an inline // eslint-disable-line immutable/no-mutation to every constructor, but it would be cleaner if this could be done via a single configuration option.

Example Here is an example of how I am using classes functionally, rather than for OO:

import Immutable from 'immutable';

class TimeInterval
{
  constructor(state) {
    this._state = state; // eslint-disable-line immutable/no-mutation
  }

  static create(t, tSession) {
    return new TimeInterval(Immutable.Map({
      t,
      tOffset: t - tSession,
      changeSets: ChangeSetsMap.create().state
    }));
  }

  get state() {
    return this._state;
  }

  get time() {
    return this._state.get('t');
  }

  get timeOffset() {
    return this._state.get('tOffset');
  }

  get changeSets() {
    return new ChangeSetsMap(this._state.get('changeSets'));
  }

  getLatestGraph(batchType = 'granular') {
    const changeSets = this.changeSets[batchType];
    if(!changeSets) {
      throw new Error('Invalid changeset batch type: ' + batchType);
    }
    return changeSets.latestGraph;
  }

  updateGraph(updater) {
    const changeSets = this.changeSets
      .update('granular', list => list.update(updater))
      .update('cascading', list => list.update(updater))
      .update('single', list => list.update(updater));
    const state = this._state.set('changeSets', changeSets.state);
    return new TimeInterval(state);
  }
}
deepu105 commented 7 years ago

Its really annoying when the rule triggers inside a constructor. May be it would be nice to have another rule to disable checking in constructor or even better to have rule option to exclude constructor or a pattern

ljharb commented 7 years ago

class MyArray extends Array { constructor() { this.length = 'mutated'; } } - you can never guarantee anything in the constructor isn't mutating an existing value or triggering a setter upstream.