mobx-react does not support "public class fields syntax" #719
Comments
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 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
|
I don't have any strong reason to change it. I just write |
Feel free to make a PR with such warning ;) You are probably pretty much alone on this, never seen anyone else doing it. |
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 |
It's not that uncommon: A bit of an explanation: Mobx have to "patch" certain lifecycle methods (including @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. We could warn/throw on missing const baseRender = target.render;
if (typeof baseRender !== 'function') {
throw new Error("...missing render, don't use field initializers for lifecycle methods ...");
} |
I think such warning is a reasonable middle ground, especially since it's not that uncommon as I thought (very surprised by it). |
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. |
@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. |
Btw there is a PR for |
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. |
I think complication / wrapping classes is not worth the effort. I think using 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. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions. |
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:
And now, this is NOT going to work:
Hope it could work soon.
The text was updated successfully, but these errors were encountered: