Issue #839: Re-add isMobXReactObserver property and perform check on class components by ynejati · Pull Request #843 · mobxjs/mobx-react

ynejati · 2020-02-28T13:14:18Z

Fixes #839

Added/updated unit tests
Updated changelog
Updated README if applicable

ynejati · 2020-02-28T13:18:21Z

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 · 2020-02-28T14:00:17Z

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 · 2020-02-28T17:08:50Z

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. ***@***.***> 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 <#843?email_source=notifications&email_token=ADAQOYZPA3WES2LU2B6ULDDRFEKHFA5CNFSM4K5QCYAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENITNMI#issuecomment-592524977>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADAQOYY5TERFRZCDAATKPFTRFEKHFANCNFSM4K5QCYAA> .

ynejati · 2020-02-29T14:19:34Z

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?

danielkcz

Looks better, although I am still not happy that functional components won't have this check. I would like to see it unified.

ynejati · 2020-03-28T14:27:35Z

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 · 2020-04-22T18:20:01Z

@ynejati Are you willing to continue on this PR?

ynejati · 2020-04-28T12:12:34Z

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

danielkcz · 2020-04-28T18:29:06Z

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

ynejati · 2020-04-29T12:16:15Z

Bummer. Okay.

ynejati · 2020-05-02T12:42:30Z

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

…ck on class components.

danielkcz · 2020-05-02T12:47:04Z

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.

- Update mobx-react-lite dependency version - Import observer batching in jest setup config

ynejati · 2020-05-05T12:23:22Z

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

danielkcz

Thanks, just one last change, please.

…rsion 2.0.6

danielkcz · 2020-05-06T14:03:24Z

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

danielkcz · 2020-07-27T06:43:34Z

    const target = componentClass.prototype
+
+    if (componentClass[mobxObserverProperty]) {
+        const displayName = getDisplayName(this)


@ynejati Looks like the issue #887 is related to this change. What is the idea behind using this in here? It's going to be undefined every time.

This can't be correct, as this isn't bound at this (lol) point, I think componentClass.displayName || componentClass.name will do the trick here

In Javascript "strict" mode this will die hard iirc

Well, that's silly. Surprised that it's taken this long to catch it. How embarrassing. I guess the test failed to catch it as well. I will submit a PR later today if someone doesn't get to it before me.

target seems to work too

Test output before (using this):

Test output after (using target):

ynejati changed the title ~~Issue #839: Re-add isMobXReactObserver property and perform check on …~~ Issue #839: Re-add isMobXReactObserver property and perform check on class components. [WIP] Feb 28, 2020

danielkcz reviewed Mar 2, 2020

View reviewed changes

Comment thread src/observerClass.ts Outdated

Comment thread src/observerClass.ts Outdated

ynejati changed the title ~~Issue #839: Re-add isMobXReactObserver property and perform check on class components. [WIP]~~ Issue #839: Re-add isMobXReactObserver property and perform check on class components Mar 28, 2020

mweststrate reviewed Apr 19, 2020

View reviewed changes

Comment thread src/observerClass.ts Outdated

ynejati force-pushed the ismobxreactobserver branch from 51c87dd to ab88a4e Compare April 28, 2020 12:33

ynejati added 6 commits May 2, 2020 05:43

Update size limit config to 4.1

3f945f8

Issue mobxjs#839: Re-add isMobXReactObserver property and perform che…

feed82a

…ck on class components.

Better observer class warnings, WIP

999ae02

Add __DEV__ as global in lint config

97e06aa

Better logging, fix tests, and clean up

ea78b57

Log redeclared observer warning in both production and development

27c2f92

Fix broken tests

3f6a121

- Update mobx-react-lite dependency version - Import observer batching in jest setup config

ynejati force-pushed the ismobxreactobserver branch from ab88a4e to 3f6a121 Compare May 2, 2020 13:30

danielkcz suggested changes May 5, 2020

View reviewed changes

Comment thread package.json Outdated

Semantic versioning greater or equal to mobx-react-lite dependency ve…

cc63e26

…rsion 2.0.6

danielkcz approved these changes May 6, 2020

View reviewed changes

danielkcz merged commit 376bff7 into mobxjs:master May 6, 2020

danielkcz reviewed Jul 27, 2020

View reviewed changes

Uh oh!

Conversation

ynejati commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ynejati commented Feb 28, 2020

Uh oh!

danielkcz commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ynejati commented Feb 28, 2020 via email

Uh oh!

ynejati commented Feb 29, 2020

Uh oh!

danielkcz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ynejati commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

danielkcz commented Apr 22, 2020

Uh oh!

ynejati commented Apr 28, 2020

Uh oh!

danielkcz commented Apr 28, 2020

Uh oh!

ynejati commented Apr 29, 2020

Uh oh!

ynejati commented May 2, 2020

Uh oh!

danielkcz commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ynejati commented May 5, 2020

Uh oh!

danielkcz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danielkcz commented May 6, 2020

Uh oh!

danielkcz Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mweststrate Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mweststrate Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

ynejati Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

ynejati Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

ynejati Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ynejati commented Feb 28, 2020 •

edited

Loading

danielkcz commented Feb 28, 2020 •

edited

Loading

ynejati commented Mar 28, 2020 •

edited

Loading

danielkcz commented May 2, 2020 •

edited

Loading

danielkcz Jul 27, 2020 •

edited

Loading

mweststrate Jul 27, 2020 •

edited

Loading