[Improvement] useObserver hook

[elderapo]

I believe the way mobx-react works with react hooks could be drastically improved. I think a single hook could remove most of the mobx-react-lite API.

The idea is to pass store or any observable to useObservable hook which should return proxied observable object. Then in the proxy get handler we can track "observable accesses" to determine when component should be rerendered. I managed to create a simple proof of concept:

const useObservable = <T extends object>(store: T): T => {
  const forceUpdate = useForceUpdate();

  const reaction = useRef<Reaction | null>(null);
  if (!reaction.current) {
    reaction.current = new Reaction(`useObservable`, () => {
      forceUpdate();
    });
  }

  const dispose = () => {
    if (reaction.current && !reaction.current.isDisposed) {
      reaction.current.dispose();
      reaction.current = null;
    }
  };

  useUnmount(() => {
    dispose();
  });

  const proxiedStore = useMemo(
    () =>
      new Proxy(store, {
        get(target, key: keyof typeof target) {
          let val: unknown;

          if (reaction.current) {
            reaction.current.track(() => {
              val = target[key];
            });
          }

          return val;
        }
      }),
    [store]
  );

  return proxiedStore;
};

It works, however, I don't know much about how mobx works internally so there might be a better way of doing it. As I said this is POC so there are most likely unhandled edge cases and improvements to be made.

I have also created a simple demo for anyone interested.

[danielkcz]

Hm, another attempt to get rid of the observer. I have seen so many around that have failed that I am already kinda skeptical it can be done :)

This approach won't work in IE11 due to lack of Proxies, that's the fact so we cannot really recommend it as a viable solution.

It would need to build more around the behavior of useObserver from the next branch of mobx-react-lite to cover for memory leaks with Concurrent mode.

Besides a weirdness of double proxying (MobX 5 does that), I don't see a reason why this couldn't work, but I sure more experienced people will spot something :)

Edit: Ah, one possible weakness. What be surely a problem is nested properties in the store. You would need to deeply iterate the whole store, find primitives in there and wrap those. Considering some stores might be somewhat large, doing that all the time in component might be somewhat expensive.

Another point is that if you need to use multiple stores, you would need to wrap each one of them into this hook before the use.

[mweststrate]

From a quick glance: it seems it tracks only shallowly. Also, if that store is passed to some other component, the 'wrong' component will be registered as dependency (namely the parent instead of child)

…

On Thu, Nov 14, 2019 at 6:46 AM Daniel K. ***@***.***> wrote: Hm, another attempt to get rid of the observer. I have seen so many around that have failed that I am already kinda skeptical it can be done :) This approach won't work in IE11 due to lack of Proxies, that's the fact so we cannot really recommend it as a viable solution. It would need to build more around the behavior of useObserver from the next branch of mobx-react-lite to cover for memory leaks with Concurrent mode. Besides a weirdness of double proxying (MobX 5 does that), I don't see a reason why this couldn't work, but I sure more experienced people will spot something :) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#800?email_source=notifications&email_token=AAN4NBE4LEAZFTPU5FPBLITQTTX2XA5CNFSM4JNFQUC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEAY42Q#issuecomment-553750122>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBGEWTY4OYB2DB5B7QTQTTX2XANCNFSM4JNFQUCQ> .

[elderapo]

@FredyC, proxies are already required in mobx@5 so it this shouldn't be an issue.

Also, this hook could accept single observable const state = useObservable(someObservable) or array of them const [state1, state2] = useObservable(store1, store2). Or maybe useObservable and useObservables... not sure.

@mweststrate, as I said this was a quick POC. For this to work in the real world it would most likely require the use of deep proxy so mobx will be able to keep track of nested observable items even if the whole object (object passed to hook) is not observable itself.

Regarding the nested components issue. I think it could be solved by checking in proxy getter when in the execution process react is and based on that track value access or not.

[elderapo]

@mweststrate Just so we're on the same page. With passing stores to child you mean exactly something like this?

const Parent = () => {
  const state = useObservable(observableState);

  console.log("rendering parent");
  return <Child state={state}></Child>;
};

type ChildProps = {
  state: ObservableState;
};

const Child = (props: ChildProps) => {
  console.log("rendering child");
  return <div>child counter: {props.state.count}</div>;
};

I am not sure what should be the expected outcome here. The easiest solution would be to advise users not to pass observed objects (results of useObserver) to children components and instead use useObserver in child components like:

const Parent = () => {
  console.log("rendering parent");
  return <Child state={observableState}></Child>;
};

type ChildProps = {
  state: ObservableState;
};

const Child = (props: ChildProps) => {
  const state = useObservable(props.state);

  console.log("rendering child");
  return <div>child counter: {state.count}</div>;
};

[danielkcz]

I wonder what it would take to somehow utilize existing proxies in mobx itself. If we would be able to attach some sort of listener that would fire whenever some observable from that object is accessed, it would be super easy to just re-render component after that.

[elderapo]

If something like this was possible with mobx@5 then writing useObservable hook would be super trivial.

const state = observable({
  count: 100
});

const proxy = mobx.proxy(state); // <-- not sure about the name

proxy.on("read", () => {
  console.log("read");
});

proxy.on("write", () => {
  console.log("write");
});

proxy.value.count; // should trigger read
proxy.value.count++; // should trigger write

state.count; // should trigger read
state.count++; // should trigger write

proxy.reset();

state.count; // should not trigger read
state.count++; // should not trigger write

Of course, it would require nested proxy to handle cases like this:

class CounterStore {
  @observable
  public count: number = 100;

  @action.bound
  public incrementCount(): void {
    this.count++;
  }
}

const state = {
  a: {
    b: {
      c: new CounterStore()
    }
  }
};

mobx.proxy(state)

[mweststrate]

intercept / observe api's can do most of that already I think. That being said I don't think personally it will lead to anything better than what we now have :). We played a lot with similar ideas in the past, but the added playrules / edge cases never seemed to justify the super minor benefit of moving observer from outside inside. (Note that you still need to apply React.memo for example when moving to a hook). But obviously feel free to prove me wrong :). But I recommend to do a bunch of searching in past issues of mobx-react / mobx-react-lite repo's, it will point you at some traps you might be missing otherwise.

…

On Thu, Nov 14, 2019 at 1:12 PM Tomasz Martyński ***@***.***> wrote: If something like this was possible with ***@***.*** then writing useObservable hook would be super trivial. const state = observable({ count: 100 }); const proxy = mobx.proxy(state); // <-- not sure about the name proxy.on("read", () => { console.log("read"); }); proxy.on("write", () => { console.log("write"); }); proxy.value.count; // should trigger readproxy.value.count++; // should trigger write state.count; // should trigger readstate.count++; // should trigger write proxy.reset(); state.count; // should not trigger readstate.count++; // should not trigger write Of course, it would require nested proxy to handle cases like this: class CounterStore { @observable public count: number = 100; @action.bound public incrementCount(): void { this.count++; } } const state = { a: { b: { c: new CounterStore() } } }; mobx.proxy(state) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#800?email_source=notifications&email_token=AAN4NBFPJVT3222TAP4VIMLQTVFFNA5CNFSM4JNFQUC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEBZA6Y#issuecomment-553881723>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBGSS4KC4H6FARU7K6DQTVFFNANCNFSM4JNFQUCQ> .

[elderapo]

I created repo so it's easier to follow the progress for anyone interested.

I've managed to implement MobxProxy with deep proxy and it seems to be working. At the moment it's kind of messy because I don't really know mobx internals. There are probably lots of improvements to be made here.

Implementation of useObserver is fairly simple and straightforward. Example uses cases can be found here.

Regarding playrules and edgecases. After a brief look at mobx-react and mobx-react-lite repos I couldn't find much info about them. Care to point me to the specific issues?

[elderapo]

I've created a new repo and published it to npm as mobx-react-better-use-observable. Simple example:

import { observable, action } from "mobx";
import { useObservable } from "mobx-react-better-use-observable";
import * as React from "react";

class CounterStore {
  @observable
  public count: number = 100;

  @action.bound
  public increment(): void {
    this.count++;
  }
}

const store = new CounterStore();

const CounterDisplayerWithoutHOF = () => {
  const { count, increment } = useObservable(store);

  return (
    <div>
      <h2>Counter(without observer): {count}</h2>
      <button onClick={increment}>increment</button>
    </div>
  );
};

More examples/edge cases can be found here (source).

Implementation of MobxProxy might need a rewrite because it got a little bit messy. Will try to add some hooks/react tests later.

[danielkcz]

I believe this has been implemented by useLocalStore already. Open a new issue if you think there is still something else to this proposal.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.