Optimize reactive props #436

Strate · 2018-03-09T16:54:24Z

Particular fix for #380 #347

Achieve granular observale state/props

Technique:
Instead of having only 1 atom for whole props/state, we have N atoms for each property/state element, and extra one which track keyset change

When new props object set, we detect key change and report changed for extra atom. Also we instrument props/state object and in-place change properties to getter+setter, which uses atoms and single value storage. Also, previous props/state object should be freezed with it values and de-instrumented from getters and setters, to get lifecycle methods work.

Having extra atom, which detects keys change helps us to detect new props/state element appears.

Unfortunatelly, to make in-place instrument with props/state object, I forced to make a extra wrapper for Observer component, which calls to createElement and un-freeze it props. This is definetely not good for performance, but React.createElement freeze objects :( I think this extra wrapper get this issue back: mobxjs/mobx#405 (and many tests are failed because of this)
UPD: solution found

Comments are requested.

Computed, which are depend on component props, should be recalculated only if used props are changed, or new prop appears, or old prop disappears Test added

Achieve granular observale state/props Technique: Instead of having only 1 atom for whole props/state, we have N atoms for each property/state element, and extra one which track keys set change When new props object set, we detect key change and report changed for extra atom. Also we instrument props/state object and in-place change properties to getter+setter, which uses atoms and single value storage. Also, previous props/state object should be freezed with it values and de-instrumented from getters and setters, to get lifecycle methods work. Having extra atom, which detects keys change helps us to detect new props/state element appears.

Remove unused variable

Strate · 2018-03-09T17:48:27Z

Hmm, seems we can optimize things, if report to keysAtom only on new key appears, but not on old rey removed.

Found solution without extra component wrapper. Done with clone-if-frozen technique, and this plays well with both state and props

Strate · 2018-03-09T21:30:21Z

Found solution without extra component wrapper.
Done with clone-if-frozen technique, and this plays well with both state and props.
And all tests are good.

We can make further optimization, by tracking by extra atom only new keys, not removing, but this requires more work, and I don't know worth it is or not.

I think that there could be done some optimizations in code which make storage observable/static.

And, this code could be easily adopted to Proxy in future.

I will test this patch on my project and give some feedback in next week.

Strate · 2018-03-10T08:01:04Z

src/observer.js

+                            individualAtoms[key].reportObserved()
+                            return individualValues[key]
+                        },
+                        set: function set(v) {


I think we can make property read-only (by defining only get), this can help to make props read-only too (BC break with current react behaviour)

mweststrate · 2018-03-11T14:15:05Z

Hey @Strate , thanks! Will try to review in a few days

Strate · 2018-03-12T14:50:16Z

Caveat: if we use reaction like this:

reaction (() => this.props, props => console.log(props))

it will not react as before, it will only react on keys changed (BC break here)

mweststrate · 2018-03-23T12:44:42Z

@Strate that works for normal observable objects neither, so I think that should be ok

mweststrate · 2018-03-23T12:51:11Z

src/observer.js

+             * In-place convert properties to getter+setter
+             */
+            function convertStorageToReactive(storage) {
+                if (null == storage) {


just if (!storage) ?

mweststrate · 2018-03-23T12:52:52Z

src/observer.js

+                // we can do it safely inside componentWillMount (react won't warn us about it)
+                // btw, state object can not be cloned here, (react warns about direct state modification), however
+                // state object is not frozen, so we do not need to clone it to instrument
+                if (Object.isFrozen && Object.isFrozen(storage)) {


Hmm, if we implemented this using Proxies, it would all be lot simpler I guess? I think that could be fine as well, only introduce this feature in the next mobx-react that accompanies MobX 5

Unfortunatelly, we can't wrap state to proxy here, because react will check this and warn (proxied object !== original object). But for frozen props, of course, it could be applied.

mweststrate · 2018-03-23T12:54:16Z

src/observer.js

+                        individualAtoms[key] = new Atom("this." + propName + "." + key)
+                    }
+                    const currentKeyValue = storage[key]
+                    delete storage[key]


skip the delete, will probably make it slower, without adding value as it is immediately redefined

mweststrate · 2018-03-23T12:55:38Z

src/observer.js

+                }
+                Object.keys(storage).forEach(function(key) {
+                    if (!(key in individualAtoms)) {
+                        individualAtoms[key] = new Atom("this." + propName + "." + key)


might be simpler to just create a collection of observable.box -es? That combines an atom with a value holder

Good catch, of course boxes would be better

mweststrate · 2018-03-23T12:58:44Z

src/observer.js

+             * In-place convert getter+setter to plain property
+             */
+            function convertStorageToStatic(storage) {
+                if (null == storage) {


Strate · 2018-03-23T13:28:49Z

@mweststrate

that works for normal observable objects neither, so I think that should be ok

Yes, but right now it works differently for props & state, and this would be BC break. It hink we can just note this in UPGRADE guide

mweststrate · 2018-03-23T13:00:48Z

src/observer.js

+                        currentValue = newValue
+                        if (
+                            currentValueKeys.length !== newKeys.length ||
+                            newKeys.filter(key => currentValueKeys.indexOf(key) >= 0).length !==


use shallowEqual, its already somewhere in the package I think

mweststrate · 2018-04-30T08:49:17Z

Hey @Strate, were you going to process the above comments?

If you could rebase it on the master branch that would be great, but if it is too invasive, don't bother

Edit: master branch

Strate · 2018-05-04T19:04:16Z

Hey @mweststrate
Just started to work on it.

…eactive_props

Fix tests after merge with master

moved from atom + value to observable.box reused shallowEqual helper simplified empty storage checks

Strate · 2018-05-04T19:32:50Z

@mweststrate merged with master, also processed all above comments.

Strate · 2018-05-04T19:35:41Z

Maybe it would be better to move for new mobx4 object api? 🤔

xaviergonz · 2018-05-29T09:01:32Z

or maybe give the observer decorator an option param and let the user choose the behavior?

Strate · 2018-05-29T12:07:41Z

@xaviergonz for what reason?

xaviergonz · 2018-05-29T13:21:58Z

oh, if you don't want to break backwards compatibility, or offer it

xaviergonz

does this work if you add props dynamically? this is, on the first render props has only one key and then in the future props has 3 keys for that same component

Strate · 2018-05-30T08:21:29Z

@xaviergonz yes, it works. There is a test for that case.

xaviergonz · 2018-05-30T15:40:15Z

@Strate awesome, I can't wait to see it on mobx itself 👍

…eactive_props

Strate · 2018-06-08T08:10:03Z

Unfortunatelly, this PR is broken now, because of #496

Strate · 2018-06-08T10:19:38Z

So, we have a problem here. To get this PR work, I need to instrument props passed to component, but I can't do this because react warns that props are modified. I'm not sure that it is possible at all.

xaviergonz · 2018-06-08T11:22:13Z

can you pass to the constructor a modified props?

…

On Fri, Jun 8, 2018, 12:19 Artur Eshenbrener ***@***.***> wrote: So, we have a problem here. To get this PR work, I need to instrument props passed to component, but I can't do this because react warns that props are modified. I'm not sure that it is possible at all. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#436 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGA77F5N8PILAXPubzptKpyC41FXxLsJks5t6k_DgaJpZM4Skjo8> .

Strate · 2018-06-08T18:34:15Z

@xaviergonz I tried that solution for a while, but some tests still failing. And this solution need an extra wrapper for a component (which is performance gain)

hong1997 · 2018-09-24T03:00:28Z

So finally, is PR ‘Achieve granular observale state/props’ implemented in newest release now?

bsmith-cycorp · 2018-10-16T15:32:17Z

Is this still alive? Is the React warning a total roadblock?

Strate · 2018-10-18T07:49:57Z

@bsmith-cycorp @hong1997 this PR is still pending, because of that warning. That warning could not be just ignored, because this can break some stuff (and it definetely break, because tests are failing in this PR)

xaviergonz · 2018-10-18T17:52:17Z

I do something like this to get reactive props

import { extendObservable, isObservableProp, observable, reaction } from "mobx";
import { disposeOnUnmount } from "mobx-react";
import * as React from "react";

export abstract class ObsComponent<P = {}, S = {}> extends React.Component<P, S> {
  @observable.shallow
  obsProps = this.props;

  constructor(props: P, context?: any) {
    super(props, context);
    const obsProps: any = this.obsProps;

    disposeOnUnmount(
      this,
      reaction(
        () => this.props,
        (unobsProps: any) => {
          const extend: any = {};
          let extended = false;

          for (const propName of Object.keys(unobsProps)) {
            if (isObservableProp(obsProps, propName)) {
              obsProps[propName] = unobsProps[propName];
            } else {
              extended = true;
              extend[propName] = unobsProps[propName];
            }
          }

          if (extended) {
            extendObservable(obsProps, extend, undefined, { deep: false });
          }
        },
        {
          fireImmediately: true
        }
      )
    );
  }
}

Then I extends from ObsComponent instead of Component and use reactions over this.obsProps (or use it on the render to get less useless renders) rather than this.props

Although now that I think about it it would probably be better if it was transformed into a function like

class C extends Component {
  obsProps = observableProps(this);
}

I can make such function (plus another for state) and add it to a PR if somebody is interested

xaviergonz · 2018-10-18T18:02:46Z

The good thing of that approach is that it is not gonna break anything :)

Strate · 2018-10-19T07:27:40Z

@xaviergonz yes, some kind like your solution implemented here, but with this.props. Your solution has one huge drawback in mobx4: if you access unexisting prop in this.obsProps, and after a while prop become exists, new value will not pick up by render (or other computed)

xaviergonz · 2018-10-19T08:23:51Z

are you sure? it uses extends Observable when new properties are detected in props

Strate · 2018-10-19T08:48:24Z

@xaviergonz yes, it extends observable, and if you will read prop after extendObservable, everything gonna be okay.

Imagine this case:

class SomeComp extends ObsComponent {
  @comptued
  get someComputed() {
    return this.obsProps.prop ? 1 : 2;
  }
  render() {
    if (1 == this.someComputed) {
      return "One";
    }
    return "Two";
  }
}

Component initially renders as <SomeComp />, and then re-renders as <SomeComp prop={true} />.
In this case, your computed initially tries to get access to non-existence property, and will not re-compute after property apperas, even if extendObservable used.

If you get rid of computed in that case everything gonna to be oka, because react re-renders component if new props come.

xaviergonz · 2018-10-19T16:02:26Z

Hmm, could be achieved by proxies (but I guess that's the same mobx5 is doing internally and that's why it works)

@mweststrate is there a way to mark an observed property as "dirty" or "invalidate" it?
If so, for each new (non previosly observable) property it could run something like "invalidateProperty(obsProps, propName)" to let the computed function it should look again

On the worst case scenario I guess it is better to provide those methods for mobx5 users than not to provide them at all?

Strate · 2018-10-19T19:51:27Z

@xaviergonz in this PR I solved that kind of problem by adding extra atom, which tracks keys change. You can check this PR for implementation details.

Strate · 2018-10-19T19:53:28Z

And yes, there is many ways to achieve granular props, easies is use expr helper:

class SomeComp extends ObsComponent {
  @comptued
  get someComputed() {
    return expr(() => this.obsProps.prop) ? 1 : 2;
  }
  render() {
    if (1 == this.someComputed) {
      return "One";
    }
    return "Two";
  }
}

but main goal of this PR is to get optimized behavior out-of-the-box, without need to rewrite whole app.

mweststrate · 2019-04-17T14:23:14Z

Closing, as this gets less, not more relevant, with the direction hooks are taking

Strate · 2019-04-17T14:25:37Z

@mweststrate could you please share some more info why this is no more relevant?

bsmith-cycorp · 2019-04-17T15:45:38Z

For those landing on this ticket, here's another workaround:

function createReactiveProps<T>(componentInstance: { props: T }): T {
  let props = observable({});
	
  Object.assign(props, componentInstance.props)
  autorun(() => Object.assign(props, componentInstance.props))
  return props;
}

Usage:

@observer class MyComponent extends React.Component<Props> {

  @observable reactiveProps = createReactiveProps(this);
  ...

Strate · 2019-04-23T13:47:21Z

@bsmith-cycorp looks like your workaround would work only with mobx@5, but mobx@4. Unfortunatelly, there still exists some runtimes where mobx@4 actual (react-native & old ios devices).

And your solution includes a leak (non-disposed autorun)

Strate added 3 commits March 8, 2018 23:59

Optimize reactive props

675120b

Computed, which are depend on component props, should be recalculated only if used props are changed, or new prop appears, or old prop disappears Test added

Optimize reactive props

c4ebc8d

Remove unused variable

Optimize reactive props

2fb0d91

Found solution without extra component wrapper. Done with clone-if-frozen technique, and this plays well with both state and props

Strate commented Mar 10, 2018

View reviewed changes

Strate requested a review from mweststrate March 11, 2018 12:15

mweststrate reviewed Mar 23, 2018

View reviewed changes

mweststrate suggested changes Mar 23, 2018

View reviewed changes

mweststrate mentioned this pull request Apr 30, 2018

Using props causes reactions and computeds to trigger in undesirable ways #380

Closed

Strate added 3 commits May 4, 2018 22:09

Merge branch 'master' of github.com:mobxjs/mobx-react into optimize_r…

0f28a68

…eactive_props

Optimize reactive props

51b14b2

Fix tests after merge with master

Optimize reactive props

12ba56a

moved from atom + value to observable.box reused shallowEqual helper simplified empty storage checks

urugator mentioned this pull request May 29, 2018

changing any prop makes computed re-evaluate even if they don't use that prop directly #486

Closed

xaviergonz reviewed May 29, 2018

View reviewed changes

Merge branch 'master' of github.com:mobxjs/mobx-react into optimize_r…

1bba46e

…eactive_props

mweststrate closed this Apr 17, 2019

danielgavrilov mentioned this pull request Aug 15, 2022

fix: avoid redundant recomputation in choroplethMap on hover owid/owid-grapher#1582

Closed

Optimize reactive props #436

Optimize reactive props #436

Conversation

Strate commented Mar 9, 2018 • edited Loading

Strate commented Mar 9, 2018

Strate commented Mar 9, 2018

Strate Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

mweststrate commented Mar 11, 2018

Strate commented Mar 12, 2018

mweststrate commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strate commented Mar 23, 2018

Choose a reason for hiding this comment

mweststrate commented Apr 30, 2018 • edited Loading

Strate commented May 4, 2018

Strate commented May 4, 2018

Strate commented May 4, 2018 • edited Loading

xaviergonz commented May 29, 2018

Strate commented May 29, 2018

xaviergonz commented May 29, 2018 • edited Loading

xaviergonz left a comment

Choose a reason for hiding this comment

Strate commented May 30, 2018

xaviergonz commented May 30, 2018

Strate commented Jun 8, 2018 • edited Loading

Strate commented Jun 8, 2018

xaviergonz commented Jun 8, 2018 via email

Strate commented Jun 8, 2018

hong1997 commented Sep 24, 2018

bsmith-cycorp commented Oct 16, 2018

Strate commented Oct 18, 2018

xaviergonz commented Oct 18, 2018 • edited Loading

xaviergonz commented Oct 18, 2018

Strate commented Oct 19, 2018

xaviergonz commented Oct 19, 2018 • edited Loading

Strate commented Oct 19, 2018

xaviergonz commented Oct 19, 2018 • edited Loading

Strate commented Oct 19, 2018

Strate commented Oct 19, 2018

mweststrate commented Apr 17, 2019

Strate commented Apr 17, 2019

bsmith-cycorp commented Apr 17, 2019 • edited Loading

Strate commented Apr 23, 2019 • edited Loading

Strate commented Mar 9, 2018 •

edited

Loading

Strate Mar 10, 2018 •

edited

Loading

mweststrate commented Apr 30, 2018 •

edited

Loading

Strate commented May 4, 2018 •

edited

Loading

xaviergonz commented May 29, 2018 •

edited

Loading

Strate commented Jun 8, 2018 •

edited

Loading

xaviergonz commented Oct 18, 2018 •

edited

Loading

xaviergonz commented Oct 19, 2018 •

edited

Loading

xaviergonz commented Oct 19, 2018 •

edited

Loading

bsmith-cycorp commented Apr 17, 2019 •

edited

Loading

Strate commented Apr 23, 2019 •

edited

Loading