Skip to content
This repository was archived by the owner on Dec 31, 2020. It is now read-only.

Re-add isMobXReactObserver property on class components #839

Closed
ynejati opened this issue Feb 14, 2020 · 5 comments · Fixed by #843
Closed

Re-add isMobXReactObserver property on class components #839

ynejati opened this issue Feb 14, 2020 · 5 comments · Fixed by #843
Labels
enhancement has PR Has PR, so will be fixed soon

Comments

@ynejati
Copy link
Contributor

ynejati commented Feb 14, 2020

Hey guys,

I'm running into a stack overflow and not sure what to make of it. Making an issue here in case it happens to evolve into something.

Ends up in the endless reactive render cycle. Maybe allowStateChanges is the culprit here, or more like I'm doing something wrong? Maybe both?

For limited context, I have a page with a container (which is an observer) with easily hundreds of child containers (which are also observers) and where each child container has multiple labels (also observers).

The browser logs state that the overflow is coming from Label.render(), but might be deceptive. Started noticing it after upgrading to v6, but can't say if it's the actually cause.

Going to try downgrading to 5 in attempts to isolate, and will update here. And/or trying to reproduce this with minimal noise that shareable.

Happy to provide more context within limits, just let me know.

(anonymous) (mobx-react.umd-6.1.4.js:formatted:189)
trackDerivedFunction (mobx.umd-5.14.2.js:766)
Reaction.track (mobx.umd-5.14.2.js:1794)
u (mobx-react.umd-6.1.4.js:formatted:187)
(anonymous) (mobx-react.umd-6.1.4.js:formatted:201)
r.render (mobx-react.umd-6.1.4.js:formatted:203)
allowStateChanges (mobx.umd-5.14.2.js:976)
(anonymous) (mobx-react.umd-6.1.4.js:formatted:189)
trackDerivedFunction (mobx.umd-5.14.2.js:766)
Reaction.track (mobx.umd-5.14.2.js:1794)
u (mobx-react.umd-6.1.4.js:formatted:187)
(anonymous) (mobx-react.umd-6.1.4.js:formatted:201)
r.render (mobx-react.umd-6.1.4.js:formatted:203)
allowStateChanges (mobx.umd-5.14.2.js:976)
(anonymous) (mobx-react.umd-6.1.4.js:formatted:189)
trackDerivedFunction (mobx.umd-5.14.2.js:766)
Reaction.track (mobx.umd-5.14.2.js:1794)
u (mobx-react.umd-6.1.4.js:formatted:187)
(anonymous) (mobx-react.umd-6.1.4.js:formatted:201)
r.render (mobx-react.umd-6.1.4.js:formatted:203)
allowStateChanges (mobx.umd-5.14.2.js:976)
(anonymous) (mobx-react.umd-6.1.4.js:formatted:189)
trackDerivedFunction (mobx.umd-5.14.2.js:766)
Reaction.track (mobx.umd-5.14.2.js:1794)
u (mobx-react.umd-6.1.4.js:formatted:187)
(anonymous) (mobx-react.umd-6.1.4.js:formatted:201)
r.render (mobx-react.umd-6.1.4.js:formatted:203)
finishClassComponent (react-dom.development-16.11.0.js:18597)
updateClassComponent (react-dom.development-16.11.0.js:18550)
beginWork$1 (react-dom.development-16.11.0.js:20307)
beginWork$$1 (react-dom.development-16.11.0.js:25860)
performUnitOfWork (react-dom.development-16.11.0.js:24808)
workLoopSync (react-dom.development-16.11.0.js:24784)
performSyncWorkOnRoot (react-dom.development-16.11.0.js:24383)
(anonymous) (react-dom.development-16.11.0.js:12326)
unstable_runWithPriority (react.development-16.11.0.js:3096)
runWithPriority$2 (react-dom.development-16.11.0.js:12276)
flushSyncCallbackQueueImpl (react-dom.development-16.11.0.js:12321)
flushSyncCallbackQueue (react-dom.development-16.11.0.js:12309)
batchedUpdates$1 (react-dom.development-16.11.0.js:24505)
reactionScheduler (mobx.umd-5.14.2.js:1904)
runReactions (mobx.umd-5.14.2.js:1880)
endBatch (mobx.umd-5.14.2.js:1580)
_endAction (mobx.umd-5.14.2.js:965)
executeAction (mobx.umd-5.14.2.js:919)
res (mobx.umd-5.14.2.js:904)
(anonymous) (PageStore.ts:179)
finished (PageStore.ts:179)
@mweststrate
Copy link
Member

mweststrate commented Feb 14, 2020 via email

@ynejati
Copy link
Contributor Author

ynejati commented Feb 14, 2020

Okay, thanks. I'll keep trying to isolate and see what I can come up with. Hopefully it is just that I'm doing something wrong, and not an issue of the lib.

@ynejati
Copy link
Contributor Author

ynejati commented Feb 22, 2020

Not a MobX problem, or an actual cycle. Simply creating too many initialization invocations on startup as is necessary of an extremely complex application. Exploring and applying asynchronous solutions and simplification will solve this problem. Closing this issue.

@ynejati ynejati closed this as completed Feb 22, 2020
@ynejati
Copy link
Contributor Author

ynejati commented Feb 28, 2020

Reopening issue. It was caused by wrapping observer within observer, and explains the reason for a large nested stack of reactive render initializations, i.e. Component.render -> reactiveRender -> Reaction.track -> trackDerivedFunction -> anonymous in reaction.track -> allowStateChanges, etc. Even when all observables were stripped from component renders, and were only being rendered once on mount.

Up until v6, observer component classes were modified with the property componentClass.isMobXReactObserver = true. We should consider adding this back, checking for this property in the observer function (similar to how we check for React.memo and isMobxInjector, and log any necessary error messages.

In this particular case, checking on this property to ensure that whatever component class that is provided to an HOC is an observer can cause problems, since it will return false, requiring that the component class is wrapped in observer by the HOC. The fix is to require that all component classes passed to the HOC use the @observer decorator, which is actually better anyways because there is no "magic".

A quick search, andisMobXReactObserver is not mentioned anywhere in the changelogs or readme. I assume that the removal of this was an accident?

@ynejati ynejati reopened this Feb 28, 2020
@ynejati ynejati changed the title In a reactive render cycle and not sure why Add back isMobXReactObserver property Feb 28, 2020
@ynejati ynejati changed the title Add back isMobXReactObserver property Re-add isMobXReactObserver property on class components Feb 28, 2020
ynejati added a commit to ynejati/mobx-react that referenced this issue Feb 28, 2020
@danielkcz
Copy link
Contributor

A quick search, andisMobXReactObserver is not mentioned anywhere in the changelogs or readme. I assume that the removal of this was an accident?

It probably was an accident. I would like to see a consolidated behavior for functional components too. Perhaps it might be better to start at mobx-react-lite and expose some utilitizes to be used here.

@danielkcz danielkcz added enhancement has PR Has PR, so will be fixed soon labels Apr 22, 2020
ynejati added a commit to ynejati/mobx-react that referenced this issue Apr 28, 2020
ynejati added a commit to ynejati/mobx-react that referenced this issue May 2, 2020
danielkcz pushed a commit that referenced this issue May 6, 2020
…class components (#843)

* Update size limit config to 4.1

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

* Better observer class warnings, WIP

* Add __DEV__ as global in lint config

* Better logging, fix tests, and clean up

* Log redeclared observer warning in both production and development

* Fix broken tests

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

* Semantic versioning greater or equal to mobx-react-lite dependency version 2.0.6
@github-actions github-actions bot mentioned this issue Oct 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement has PR Has PR, so will be fixed soon
Projects
None yet
3 participants