useComputed might cause some confusion

[danielkcz]

Yesterday I run into a strange issue when the computed property wasn't being recomputed when contained observables have changed.

function MyComponent() {
  const state = useObservable({ foo: 1 })
  const value = useComputed(() => {
    return state.foo + 10
  })
  return useObserver(() => (
    <div>{value}</div>
  ))
}

https://codesandbox.io/s/k9o2jl0917

One would have thought why the hell it's not working? There is an observable inside observer, it should work. Yea well, the major gotcha is that useComputed is not returning observable, but the resulting computed value itself which is no longer observable.

To make it work it's either about using observer HOC which sometimes might not work either if there are some render prop component being rendered. Another solution is basically the following.

function MyComponent() {
  const state = useObservable({ foo: 1 })
  const value = useMemo(() => mobx.computed(() => {
    return state.foo + 10
  }))
  return useObserver(() => (
    <div>{value.get()}</div>
  ))
}

I am not entirely sure how to approach this. Currently the useComputed does the call to get() so the returned value is not observable anymore. I am thinking it shouldn't probably do it and return a full computed value. Opinions?

[xaviergonz]

Makes me wonder, does it work if it is put inside the useObserver chunk?

[xaviergonz]

Another option could be to do something like

const c = useComputeds(() => ({
  get value() {...} 
}))
... 
c.value // will call get 

[danielkcz]

Makes me wonder, does it work if it is put inside the useObserver chunk?

You mean like useComputed inside the useObserver? I think that React would not like that for sure :)

Your suggestion could actually work, I will try to add test for this later and see if it works, thanks.

[xaviergonz]

Well, it won't work without changes. It would need to "transform" by wrapping it in observable.
If you want something that would work out of the box it would be

function MyComponent() {
  const state = useObservable({
    foo: 1,
    get value() {
      return this.foo + 10
    }
  })
  return useObserver(() => (
    <div>{state.value}</div>
  ))
}

[danielkcz]

@xaviergonz I surely tried that also, but there is another caveat with computed property in useObservable - when it relies on some closure value (eg. from props), it would always use the one from the time it was initialized. That's kinda a reason why useComputed has useMemo underneath so it can be refreshed when an outside value changes.

[xaviergonz]

TBH, if useObservable supported decorators you would be able to have observable values + actions + computeds all in one
e.g.

const state = useObservable({
    foo: 1,
    get value() {
      return this.foo + 10
    },
    setFoo(val) { this.foo = val }
  }, {
    setFoo: action
  })

or even split

const state = useObservable({
    foo: 1
});
const computeds = useObservable({
    get value() {
      return state.foo + 10
    },
});
const actions = useObservable({
    setFoo(val) { state.foo = val }
  }, {
    setFoo: action
  });

[xaviergonz]

why not change use observable to use a lambda then?

[xaviergonz]

const state = useObservable(() => ({
    foo: 1,
    get value() {
      return this.foo + 10 +  props.whatever
    },
    setFoo(val) { this.foo = val }
  }, {
    setFoo: action
  }))

[xaviergonz]

On a totally unrelated note, I wonder why mobx doesn't default to make functions actions by default

[danielkcz]

On a totally unrelated note, I wonder why mobx doesn't default to make functions actions by default

That's why I started #22 ;)

why not change use observable to use a lambda then?

That kinda beats the purpose of observable. You don't want to reset it on every render. It would have to be optional behavior and I think we are better of with improving useComputed than this.

[FOODy]

What if we use a reaction?

https://codesandbox.io/s/4q335zly14

function useComputed<T>(fn: () => T, dep: DependencyList = []): T {
  const [value, setValue] = useState(fn);

  useEffect(() => {
    return reaction(fn, value => {
      setValue(value);
    }, { fireImmediately: true });
  }, dep);

  return value;
}

[samdenty]

import * as mobx from 'mobx'
import { useMemo, useState, useEffect } from 'react'

export const useComputed = <T>(
  func: () => T,
  inputs: ReadonlyArray<any> = []
): T => {
  const computed = useMemo(() => mobx.computed(func), inputs)
  const [value, setValue] = useState(() => computed.get())

  useEffect(
    () =>
      mobx.reaction(
        () => computed.get(),
        value => {
          setValue(value)
        }
      ),
    [computed]
  )

  return value
}

[danielkcz]

@samdenty I don't follow what's the benefit of wrapping it to the mobx.computed. The reaction itself already behaves like a computed value because it gets a callback to provide an observable. The solution from @FOODy is more than enough imo.

Either way, both of these will surely work. It's just that making a separate reaction for each computed value feels kinda overkill. The reaction is already part of the useObserver and would take care of it. Except it's no longer observable when used.

Also if you want to use such computed value in the <Observer />, it would re-render a whole component containing the useComputed anyway which is not expected outcome.

Personally, I had no use case for useComputed so far. Perhaps it should not have been part of the package initially. Oh well :)

[samdenty]

@FredyC Reason I put it inside a computed is because in the initial render (before useEffect is called), he used useState(fn). If useComputed was used within a component that had observer, it would pick up dependencies from the fn function.

Could also use untracked, but it would still require making two calls:
one to set the initial state +
one for reaction to pick up dependencies

[danielkcz]

Closing since we have deprecated useComputed in favor of useLocalStore. Released in 1.3.0

[lennerd]

Any chance we have a useComputed hook again? I wrote one myself also working with useObserver similar to the new syntax in Vue 3:

function useComputed<T>(
  func: () => T,
  deps: ReadonlyArray<any> = []
) {
  return React.useMemo<ComputedValueObject<T>>(() => {
    const computedValue = computed(func);

    return {
      get value() {
        return computedValue.get();
      }
    };
  }, deps);
}

const App = () => {
  const inputRef = React.useRef<HTMLInputElement>();
  const todos = useObservable<Todo[]>([]);
  const pendingTodos = useComputed(() =>
    todos.value.filter(todo => !todo.done)
  );
  const doneTodos = useComputed(() =>
    todos.value.filter(todo => todo.done)
  );
  const addTodo = useAction(() => {
    if (inputRef.current.value === '') {
      return;
    }

    todos.value.push({
      task: inputRef.current.value,
      done: false
    });

    inputRef.current.value = '';
  });
  const toggleTodo = useAction(todo => {
    todo.done = !todo.done;
  });

  const renderTodo = (todo: Todo, index: number) => {
    return (
      <li key={index} onClick={() => toggleTodo(todo)}>
        <input type="checkbox" disabled checked={todo.done} />
        {todo.task}
      </li>
    );
  };

  return useObserver(() => (
    <div>
      <h4>Pending</h4>
      <ul>{pendingTodos.value.map(renderTodo)}</ul>
      <h4>Done</h4>
      <ul>{doneTodos.value.map(renderTodo)}</ul>
      <div>
        <input ref={inputRef} />
        <button onClick={addTodo}>Add todo</button>
      </div>
    </div>
  ));
};

You can find a running example here: https://codesandbox.io/s/mobx-hooks-63yr6

TBH I tried around with useLocalStore and have the feeling its syntax is actually a lot more cumbersome and error prone than using simple useObservable and useComputed hooks. (I'm also no big fan of magically transforming object properties and getters to observable and computed properties. But thats another topic.)

At the end it might be a very subjective feeling but the multiple hooks approach feels a lot easier to read to me.

What do you think?