Issue #839: Re-add isMobXReactObserver property and perform check on class components

[ynejati]

Fixes #839

• [x] Added/updated unit tests
• [x] Updated changelog
• [ ] Updated README if applicable

[ynejati]

Updated the change log. Going to need to actually increment the version, potentially add tests, and update the Readme. Looking for guidance on what you'd like me to do. Should have marked it as a draft PR, but can't backtrack, so going to mark as WIP for now.

[danielkcz]

Do not need to bump version, will happen in a release process.

I would probably wrap both of those code blocks to __DEV__ check as we don't this in production.

Adding test good idea for sure and to tweak the on that is broken as well.

Also thinking it might better to use Symbol instead of string property.

[ynejati]

Good point. Also, something just realized, if the render of a parent class is a reaction as a result of observer and that subclass overrides the render of the parent, the render is no longer reactive.

On Fri, Feb 28, 2020, 6:00 AM Daniel K. notifications@github.com wrote:

Do not need to bump version, will happen in a release process.

I would probably wrap both of those code blocks to DEV check as we don't this in production.

Adding test good idea for sure and to tweak the on that is broken as well.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/pull/843?email_source=notifications&email_token=ADAQOYZPA3WES2LU2B6ULDDRFEKHFA5CNFSM4K5QCYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENITNMI#issuecomment-592524977, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADAQOYY5TERFRZCDAATKPFTRFEKHFANCNFSM4K5QCYAA .

[ynejati]

Okay, I see now. Regarding a subclass who extends a parent class and overrides the parents reactive render. This behavior is not supported. The subclass will need to re-declare itself as an observer. That is fair. Curious, what happens to the reaction of the parent class. Is it leaked?

[ynejati]

Decided to keep it simple, and just log a warning instead of performing some hand holding. I haven't heard anyone else complain about the removal of the isMobxReactObserver string property, so we went with symbol/string util. Going to do a similar check in the lite lib next.

[danielkcz]

@ynejati Are you willing to continue on this PR?

[ynejati]

@FredyC, yep. Sorry, been MIA. These last couple of months have been weird.

[danielkcz]

Please update the size limit config to 4.1 kB. Also, some tests are failing.

[ynejati]

Bummer. Okay.

[ynejati]

I'll fix the tests, but just so you know they are also broken on the master.

[danielkcz]

Oh! That's interesting. For a while, I thought it's some failure on Coveralls side. Only now when I saw failed test-size I checked the other one too.

If you have time for it I would certainly appreciate if you can fix those. I believe it's merely about import the observer batching, probably best to do that in jest.setup.ts.

[ynejati]

@FredyC, this thing is ready when you get a chance. Thanks, man.

[danielkcz]

Thanks a lot, that was somewhat painful PR given how little it is :)