Restored mixin based implementation of observer for class based components

[mweststrate]

Proposal to revert to the classic implementation for class based observer components; by monkey patching the prototype rather than wrapping render in <Observer>

Fixes / affects #699, #692, #697, #702

[coveralls]

Coverage increased (+1.08%) to 90.691% when pulling a4099e8 on revert-observer-class into 762e3d1 on master.

[urugator]

Huh, haven't noticed that patching logic became so convoluted over time.
I know it's been mentioned many times that patching is necessary, but tbh I am not sure about specifics.
What's exactly problematic about extending the original component? Like:

class extends originalComponent {
  componentWillUnmount() {
    super.componentWillUnmount();
    // dispose 
  }
}

[mweststrate]

Yeah, it's all disposeOnUnmount's fault :-(. It introduces a plethorea of possiblities: the decorator want to set CWU, so does observer, so does the component as defined by the user (either on prototype or by using an arrow function). So the thing is monkey patches on different levels, which is especially annoying if multiple inheritance trees are involved. See also #705 that tries to simplifies things again, but breaks some cases

…

On Tue, Jun 18, 2019 at 4:30 PM urugator ***@***.***> wrote: Huh, haven't noticed that patching logic became so convoluted over time. I know it's been mentioned many times that patching is necessary, but thb I am not sure about specifics. What's exactly problematic about inheriting the original component? Like: class extends originalComponent { componentWillUnmount() { super(); // dispose } } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#703?email_source=notifications&email_token=AAN4NBDQLEV7FS6MFOS5FETP3DWRFA5CNFSM4HYZIWB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX62HJQ#issuecomment-503161766>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBCUSKW3SF7WYZSOQF3P3DWRFANCNFSM4HYZIWBQ> .

[danielkcz]

LGTM.

I am really surprised by how much hassle class support needs. Never paid much attention to it. Considering how easy is React.useEffect compared to disposeOnUnmount, people should be sprinting toward the hooks. Or running away from company that is slow on upgrades :D

[mweststrate]

Dang, that is a good point. Would you mind opening an issue for that? IIRC class components don't support this rendered-once but never comimtted behavior. But in hindsight I'm not sure that actually makes sense.

…

On Wed, Jun 19, 2019 at 10:44 AM urugator ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/observerClass.js <#703 (comment)>: > + this.name || + (this.constructor && (this.constructor.displayName || this.constructor.name)) || + "<component>" + /** + * If props are shallowly modified, react will render anyway, + * so atom.reportChanged() should not result in yet another re-render + */ + setHiddenProp(this, skipRenderKey, false) + /** + * forceUpdate will re-assign this.props. We don't want that to cause a loop, + * so detect these changes + */ + setHiddenProp(this, isForcingUpdateKey, false) + + // wire up reactive render + const baseRender = render.bind(this) doesn't need the ugly component-might-not-have-been-committed hack Are you sure? I think the reaction would have to be created in componentDidMount (similary to inside useEffect). Check https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#adding-event-listeners-or-subscriptions : Specifically: async rendering (where rendering might be interrupted before it completes, causing componentWillUnmount not to be called). — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#703?email_source=notifications&email_token=AAN4NBGQ4373CZXATUKKBD3P3HWXNA5CNFSM4HYZIWB2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB37GQ3I#discussion_r295185176>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBFWZZRFHR6Y3TKY6V3P3HWXNANCNFSM4HYZIWBQ> .

[urugator]

Well technically there is a one already #645 :D

[mweststrate]

Fair point!

…

On Wed, Jun 19, 2019 at 11:44 AM urugator ***@***.***> wrote: Well technically there is a one already #645 <https://github.com/mobxjs/mobx-react/issues/645> :D — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#703?email_source=notifications&email_token=AAN4NBDBPM3SPTPXUPR2DL3P3H5XTA5CNFSM4HYZIWB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYBKA3Y#issuecomment-503488623>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBGFWUGW4VGTPIH6333P3H5XTANCNFSM4HYZIWBQ> .

[urugator]

And I have another bad news ... this also affects disposeOnUnmount... actually @disposeOnUnmount is conceptually completely wrong, because constructors shouldn't contain side effects... all side effects should be in cDM or cDU.

[mweststrate]

Yeah I think we have to work to a hard split in the future: If you want to use concurrent mode, limit yourself mobx-react-lite. If that is too hard of a split, we could make v7 a minimal class implementation (as proposed already) without all the other shizzle: Provider / inject / propTypes / disposeOnUnmount

…

On Wed, Jun 19, 2019 at 11:53 AM urugator ***@***.***> wrote: And I have another bad news ... this also affects disposeOnUnmount... actually @disposeOnUnmount is conceptually completely wrong, because constructors shouldn't contain side effects... all side effects should be in cDM or cDU. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#703?email_source=notifications&email_token=AAN4NBB6CJT24I4RJC5Z4LDP3H62PA5CNFSM4HYZIWB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYBK2ZI#issuecomment-503491941>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN4NBCBX3ZIBMSVCDDGNILP3H62PANCNFSM4HYZIWBQ> .