mobx-react does not support "public class fields syntax"

[jim-king-2000]

We are using "public class fields syntax" everywhere in current react projects. We hope mobx-react could support it.

So, now this is going to work:

@observer
export default class extends Component {
  render() {
    return ...;
  }
}

And now, this is NOT going to work:

@observer
export default class extends Component {
  render = () => (<>...</>);
}

Hope it could work soon.

[danielkcz]

Hm, can you please elaborate what is your reason to use the second variant? I mean it does make sense for lifecycle and other methods, but since the render is called only by React, there is no need to use such syntax.

Either way, I don't think we are going to change that. There is too many variables to this whole thing to make it work and supporting your edge is not our priority. If it would be a major request from the community, we would certainly consider it.

Or if you can figure out how to fix the use case without breaking tests, feel free to submit a PR.

mobx-react/src/observerClass.js

Line 32 in 9c9c482

const baseRender = target.render

[jim-king-2000]

I don't have any strong reason to change it. I just write render = () => (); in my project before introducing mobx. When I have to add mobx and mobx-react, I find that the reaction is not triggered. I spend several minutes finding that "obersever" does not support "public class fields syntax".
I don't know how to fix the case without breaking tests. And I'm OK if it won't be fixed. I only suggest that there should be a warning when I mix "obersever" and "public class fields syntax".

[danielkcz]

I only suggest that there should be a warning when I mix "obersever" and "public class fields syntax".

Feel free to make a PR with such warning ;) You are probably pretty much alone on this, never seen anyone else doing it.

[danielkcz]

FYI ... using class fields syntax you are deliberately deoptimizing your components. Whenever you use class fields syntax, that function is copied on the instance. In contrast regular class method lives on the prototype and is reused by every instance. So you are increasing the memory footprint of the whole application because of this unusual decision.

class UsingClassMethod {
  render() {
  }
}

class UsingFieldSyntax {
  render = () => {
  }
}

const methodA = new UsingClassMethod()
const methodB = new UsingClassMethod()
const fieldA = new UsingFieldSyntax()
const fieldB = new UsingFieldSyntax()

console.log(methodA.render === methodB.render) // true
console.log(fieldA.render === fieldB.render) // false

[urugator]

You are probably pretty much alone on this

It's not that uncommon:
https://github.com/mobxjs/mobx-react/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aclosed+arrow+

A bit of an explanation: Mobx have to "patch" certain lifecycle methods (including render).
The problem is that fields are not part of the prototype (class definition).
Fields doesn't exist anywhere until the class constructor is invoked.
So when you use field initializer instead of prototype's method, observer patches the non-existent render method on prototype and later when the constructor is invoked, the render method is replaced by your non-patched arrow function.
It's exactly like doing so:

@observer
export default class extends Component {
  constructor() {      
    // defined on instance
    // replaces render
    this.render = () => { /*... */ };
  }
  // defined on prototype, gets patched by observer
  render() {    
    return ...;
  }
}

In order to support this (or just to warn), we would have to move the patching logic at the end of the constructor, which is impossible without creating new constructor, which is an equivalent of creating new class.
Or possibly by redefining the lifecycle method as magical getter/setter, which would be really nasty.

We could warn/throw on missing render on prototype, however we can't do that for other lifecycle methods, as they're optional:

const baseRender = target.render;
if (typeof baseRender !== 'function') {
  throw new Error("...missing render, don't use field initializers for lifecycle methods ...");
}

[danielkcz]

I think such warning is a reasonable middle ground, especially since it's not that uncommon as I thought (very surprised by it).

[jim-king-2000]

Thank @FredyC and @urugator. Since "this.func" (which func is a normal method of a class) is a normal function which does NOT bind to this, we are using class fields everywhere in React projects, especially in event handling. When mixed with render(), there would be two kinds of method definition.

class X extends Component {

  onClick = e => console.log(e); // class field

  render() { // normal method
    return <Button onClick={this.onClick} />;
  }

}

My OCD obligates me to use only ONE style. That's why this thread begins.

I didn't expect that the implementations (including the warning one) are so tricky. So, at least, a text warning should be added in the README.md of the project so that I could spend less time recovering from the silent failure.

[danielkcz]

@jim-king-2000 Would you mind creating PR? The README warning is certainly useful, but adding it to the code as proposed by @urugator is also easy and will help more people for sure.

[urugator]

Btw there is a PR for eslint-plugin-react that you might be interested in:
jsx-eslint/eslint-plugin-react#1980

[jim-king-2000]

Hi @FredyC , I never read the code of mobx-react and have 0 knowledge of the fix. I'm afraid that I'm not the appropriate person to send the pull request.

[mweststrate]

I think complication / wrapping classes is not worth the effort. I think using render as arrow function is best practice anyway, as it completely unnecessary creates a closure on every instance of a component, rather than one on the prototype which is shared by all instances.

The patching logic is quite complicated already, I wouldn't touch it for this. Just throwing an error if render is not defined on the proto as proposed seems the best solution here.