Why we dropped useAsObservableSource and useLocalStore

[SidKwok]

✋✋✋ The original issue below doesn't represent true reasons. The important discussion starts at #2539

---

Intended outcome

Not trigger warning.

Actual outcome

warning from react:

index.js:1 Warning: Cannot update a component (`wrappedComponent`) while rendering a different component (`wrappedComponent`). To locate the bad setState() call inside `wrappedComponent`, follow the stack trace as described in https://fb.me/setstate-in-render
    in wrappedComponent (created by Context.Consumer)
    in Route (at App.tsx:48)
    in Switch (at App.tsx:47)
    in div (at App.tsx:46)
    in App (at App.tsx:60)
    in Router (created by HashRouter)
    in HashRouter (at App.tsx:59)
    in Unknown (at src/index.tsx:7)

How to reproduce the issue

Try to run this code

import React from 'react';
import { observer, useLocalStore } from 'mobx-react-lite';
import { useHistory, Route, Switch, HashRouter } from 'react-router-dom';

const context = React.createContext(null);

const Com = observer(() => {
  const store = React.useContext(context) as any;
  return (
    <div>
      <p>{store.str}</p>
      <button onClick={store.go}>
        test
      </button>
    </div>
  );
});

const Page = observer(() => {
  const history = useHistory();
  const storeSource = {
    history,
    search: history.location.search
  };
  const store = useLocalStore(source => ({
    get str() {
      return source.search
    },
    go() {
      source.history.replace(
        '/test?a=1'
      );
    }
  }), storeSource);
  return (
    <context.Provider value={store as any}>
      <div>
        <Com />
      </div>
    </context.Provider>
  );
});

function App() {
  return (
    <div className="App">
      <Switch>
        <Route
          path="/test"
          component={Page}
          exact
        />
      </Switch>
    </div>
  );
}

export default () => (
  <HashRouter>
    <App />
  </HashRouter>
);

``

## Versions

react: 16.13.1
react-dom: 16.13.1
react-router-dom: 5.1.2
mobx: 5.15.4
mobx-react-lite: 2.0.4

[nulladdict]

I managed to strip down example to bare React + mobx-react-lite sandbox link
Clicking increment first time indeed produces the warning

I'm pretty sure this happens due to the assignment during render in this line which triggers reaction and eventually setState

I tried moving this assignment into the useEffect which does seem to work, this version runs without warnings:

React.useEffect(() => {
    runInAction(() => {
        Object.assign(res, current)
    })
})

However this increases render count and breaks some tests. I don't have enough inside knowledge about react-mobx-lite to understand if this behaviour is good or bad and might need consulting @FredyC

I've created a draft PR to try to fix this issue. Also while running tests for it I found a potential bug in useAsObservableSourceInternal implementation, which I'll describe in the PR

[danielkcz]

To be honest, I am not sure either. This new flashy warning is causing havoc all around and it's more confusing than helpful in my opinion.

I'm pretty sure this happens due to the assignment during render in this line which triggers reaction and eventually setState

That sounds safe to me, even if observable changes during render phase, the reaction and forceUpdate are still happening a bit later inside the useEffect. There is no setState happening during the render phase afaik.

It's definitely curious why your fix helped to remove that warning. Either it's some coincidence or there is actually something it. My expertise isn't that great either.

[danielkcz]

Wait, I totally missed a reproduction from @SidKwok before, sorry about that. But looking into that, there is something smelly in here.

const Parent = observer(() => {
  const [value, setValue] = useState({ current: 0 });
  const store = useLocalStore(
    source => ({
      get current() {
        return source.current;
      },
      increment() {
        setValue(prev => ({ current: prev.current + 1 }));
      }
    }),
    value
  );
  return <Child store={store} />;
});

First of all, why are you combining useState and useLocalStore? That feels like unnecessary complication without any benefit to it. However, what's more important, why do you call setValue inside the increment action? That's conceptually all wrong. Actions should modify observables, not the outside state value. This is most likely the source of that warning.

[nulladdict]

Also it is curious that the warning shows up only for the first click but not for any subsequent clicks

Regarding useAsObservableSourceInternal, I can submit a test case where source argument for useLocalStore changes from {} to undefined which causes useLocalStore to break in a different PR

[danielkcz]

Regarding useAsObservableSourceInternal, I can submit a test case where source argument for useLocalStore changes from {} to undefined which causes useLocalStore to break in a different PR

Well, that's more of the bug in user code. The useAsObservableSource is built to always expect the same structure. I guess we can do DEV warning, but it's definitely a code smell.

[nulladdict]

Also I do agree mutating something outside of observable scope which needs to observed for some reason is conceptually wrong

[danielkcz]

@SidKwok closing the issue as it seems that your pattern is simply wrong. I will reopen if you have something else to add here.

[SidKwok]

Wait, I totally missed a reproduction from @SidKwok before, sorry about that. But looking into that, there is something smelly in here.

@FredyC
The case you shown is not my reproduction. My case is combining history.replace and useLocalStore. You can look into this link: https://codesandbox.io/s/misty-wave-tkycp?file=/src/App.js

[SidKwok]

@FredyC
If the pattern is wrong, how can the state search produced by history be observed in this case? Is there a best practice?

[danielkcz]

You are right there is something strange to your use case. I stripped down your example to remove any "weird patterns" and the issue can still be reproduced.

https://codesandbox.io/s/quirky-taussig-qfwyc?file=/src/App.js

It does really seem that useAsObservableSource mutation in the render phase is problematic here.

The strangest part is that it does happen only when location.search is an empty string. Whenever some value is there (even after reload), the warning is not happening.

@mweststrate Do you have some ideas here? There was attempted PR mobxjs/mobx-react-lite#280 to run useAsObservableSource inside the useEffect but it breaks some tests, mainly about an increased number of re-renders. What's more important that I am failing to see what's so special about this use case that it causes that warning.

[danielkcz]

Digging further and I removed react-router from the equation and the issue can be still reproduced even more reliably after every page reload (because the empty string is initial value).

https://codesandbox.io/s/festive-currying-498ny?file=/src/App.js

It almost seems that MobX does something special when an empty string is passed to observable. But that doesn't make any sense 😵.

[nulladdict]

@FredyC
If the pattern is wrong, how can the state search produced by history be observed in this case? Is there a best practice?

I think the general approach is to useLocation which subscribes to location changes and should re-render if search is changed

[danielkcz]

@nulladdict That's pretty much irrelevant, the useLocation is just a different export of the same context. It might make more sense code-wise, but doesn't help anything actually.

[nulladdict]

What I meant is that it’s probably better to not observe state managed by something else, just like in my useState example, and just use that state directly

Also empty string probably has nothing to do with a warning, since it’s still reproduces with a number in my example. I believe the area of the problem is somewhere in initialization and first updating of observable

[danielkcz]

So Michel had a look at it and the conclusion is that it seems like a false negative for that warning. There is technically nothing wrong with the pattern I've outlined in previous sandbox.

If we would go through modifying the useAsObservableSource and adding extra useEffect, we get extra re-renders for other people. That's a bad idea just for an edge case warning.

If the warning bothers you, it would be better to avoid MobX in this case because there doesn't seem to be an added value for you. Try to rethink the approach. Basically what @nulladdict said applies here. Consume useLocation (or whatever hook) directly. If you want your own abstraction, make a custom hook.

The golden rule is that you should pick the right tool for the job, not to force a single tool for every job.

[mweststrate]

Yeah, it is clear to me how the warning is caused :) It just makes me wonder, is asAsObservableSource the right solution, or do we need a different abstraction (e.g. a hoc, wrapper, dunno). That is why I'm interested in more real life use cases like the router one, rather than minimal repo's (which in turn are better to verify the solution). The risk with any useEffect based solution is that it might easily cause double renders. That in turn is probably fixable, just want to zoom out a bit first and make sure, before we start piling fixes / workarounds, that we're working in the right direction.

…

On Thu, Aug 6, 2020 at 10:04 AM Björn Zeutzheim ***@***.***> wrote: @mweststrate <https://github.com/mweststrate> as for a mobx-react-lite related example you can check my comment at the top <https://github.com/mobxjs/mobx-react-lite/issues/274#issuecomment-652958242> and this comment <https://github.com/mobxjs/mobx-react-lite/issues/274#issuecomment-653524231> for a general demonstration of where this problem originates from. To sum it up, react now says that calling setState of a different component from within the rendering of one component is regarded as an error. It still works right now, but I guess they did this because it will fail in the future with react concurrent mode. Right now this error will always occurr when the observable from useAsObservableSource is used in any other component than the current. This is because inside the hook it will update the observable object and if any other component depends on one of those values, it will be forceUpdated via setState while still rendering the current component. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/mobxjs/mobx-react-lite/issues/274#issuecomment-669807151>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBFFZT5F4HO7T2XARMDR7JWYPANCNFSM4MD5D7QQ> .

[urugator]

@mweststrate There is a double render already, currently a setState is scheduled during render, causing another one ... however in case of setState react can probably prevent double rendering of other affected components, while useEffect will wait for their render too (not sure).

[mweststrate]

Yeah, the biggest problem is here indeed that unlike with classes, we don't have a this that can own an observable props object that is / can be updated outside render. Random thought: maybe it might be interesting to see if we can make an api where this asObservabelSource is basically pulled outside the render. E.g.

export const Comp = observer(props => /* constract an observable object here */, observableProps => {
  // normal render
})

or make the props directly observable directly anyway in the HoC. There was a problem with that, but I don't remember anymore what exactly :) I'm going on holiday tomorrow, but I see if I can recall it after that.

[urugator]

Not sure if it helps anything, because it's the local store that is passable around and contains computeds that depends on this source (observableProps).
I think we shouldn't reduce the problem to observable props, which was mainly to allow the use of props in local computeds/reactions without causing staleness.
The goal here is to transform something non-observable to observable so it can be later used as such, even in different components.
It's more akin to getDerivedStateFromProps, but you want to get state that is observable.
The analogy of getDerivedStateFromProps in functional components is calling setState during render (at a cost of double render and temporal inconsistentency of values), which is basically what we do (our state is synchronous, so no inconsistency).
The ideal situation would be:
When invalidating currently rendered component call setState EDIT do not call anything (there is no point, it's already rendering and data are updated).
When invalidating other component during render, defer the update to useLayoutEffect, but in case the component starts rendering before this layout effect is invoked, dequeue the update.
If an implementation is possible, it will be probably very fragile.

To be completely clear I don't like useAsObservableSource/useLocalStore I would get rid of them immediately, I am just trying to be constructive.

Enjoy your holiday :)

[olee]

The risk
with any useEffect based solution is that it might easily cause double
renders

I think deferring the update into a useEffect will not have any impact, because afaik right now react internally does the same thing this patch does manually.
If you call setState on another component B from the currently rendering one A, the error is logged but component B is still queued to render after component A.
This fix would then result in the same amount of rendering and just follows the react semantics to properly call setState not during the rendering of another component.

It just makes me
wonder, is asAsObservableSource the right solution, or do we need a
different abstraction

There might be an alternative just for useAsObservableSource. Something like this maybe?

import React from 'react';
import { observable } from 'mobx';

export interface WithObservablePropsProps<T extends Record<string, any>> {
    observableProps: T;
}

export default function withObservableProps<T extends Record<string, any>>(Component: React.ComponentType<WithObservablePropsProps<T>>) {
    const result: React.ComponentType<T> = function WithObservableProps(props: T) {
        const observableProps = React.useState(() => observable(props))[0];
        React.useEffect(() => {
            Object.assign(observableProps, props);
        });
        return <Component observableProps={observableProps} />;
    };
    return result;
}

And then using it like this:

const MyComponent = observer(({ observableProps }: { observableProps: { num: number; str: string; }; }) {
    return (
        <div>{observableProps.num} / {observableProps.str}</div>
    );
});

const ComponentWithObservableProps = withObservableProps(MyComponent);

/*
type of ComponentWithObservableProps is
const ComponentWithObservableProps: React.FunctionComponent<{
    num: number;
    str: string;
}>
*/

However there are a few cons with this solution:

• You have to wrap your component (who still want's to use HOCs since hooks are here anyway??)
• Increases component nesting
• It only works as a replacement for useAsObservableSource.
  The other solution would also solve the problem of modifying other stores passed from context etc. in the rendering function.
  eg stuff like running if (someCondition) myGlobalStore.prop = newValue; inside a currently rendering component

[urugator]

I think deferring the update into a useEffect will not have any impact, because afaik right now react internally does the same thing this patch does manually.

I would assume that react schedules setState immediately (at least for current component), while effects wait for the DOM, but really dunno.
But I agree the extra update in effect is fine, because there is simply no other place where we could schedule this update (and we can prevent updating components that were already rendered before effect)
Not sure if it can cause some ugly waterfall: effect -> render -> effect -> render -> effect.
However I am not convinced that the extra complexity is worth it - deriving observables from props should be an edge case (even more generally deriving state from props is an edge case). People should be discouraged from doing so and there are perhaps less optimal, but sufficient ways to handle this without useAsObservableSource.

Something like this maybe?

We would have to move memo from observer to withObservableProps - or rather withObservableProps would use useObserver, therefore no extra nesting.
However I am more concerned about whether it's actually useful ... you can use these observableProps in useLocalStore's computed, but that's pretty much it ... so you itroduce extra render with effect so you can save another with computed.
I am actually worried people would start misusing it in all sorts of ways.

Another thing I am a bit worried: the props and observableProps are temporarily out-of-sync.
It doesn't seem to be a problem, because HOC only passes the "delayed" observableProps, but I think it can still cause some issues:
Eg the prop value could come from some observable store, then you obtain the store in Child component and the prop value doesn't match with the store value.
But to be fair I think something like this can already happen even without defering the update to effect.

Deleted the comment above, realized it can't work :D

[olee]

I would assume that react schedules setState immediately (at least for current component), while effects wait for the DOM, but really dunno.

For the current one yes, but we are talking about other components - in this case from what I can infer, react currently schedules them to render after the current one.

Not sure if it can cause some ugly waterfall: effect -> render -> effect -> render -> effect.

I think this might be possible, but I think it would only happen with code which would cause the same problem even with the current implementation (ie. modifying an observable value in a component during rendering which at the same time relies on this value):

const store = observable({ value: 0 });
const FailComponent = observer(() => {
  store.value += 1;
  return <div>Value = {store.value}</div>;
});

I am actually worried people would start misusing it in all sorts of ways.

As I said in my comment above - that this was just an example and this solution would have quite a few cons - so I also definitely would never use this solution.
I only posted it as an inspiration to think about so maybe someone else has a better idea based on this (though I don't really think there is any value in this).

Another thing I am a bit worried: the props and observableProps are temporarily out-of-sync.

Would not be a problem, because the new values would be applied in a useEffect which would keep the changes still in sync in regards to the whole rendering process.

---

So to get back on topic: Are there so far any arguments against going forward with solution proposed in the PR mobxjs/mobx-react-lite#299 ?

[urugator]

Would not be a problem, because the new values would be applied in a useEffect

It may break some assumptions in the render function ... what usually happens (to me) is cannot read property of undefined, because on the first render something isn't initialized and therefore must be accessed conditionally. But I can also imagine it breaking conditionally invoked effect, memo, etc.
Also you need useLayoutEffect, otherwise you could actually see the incorrect output.

so far any arguments

My main argument is againts useAsObservableSource, which I have expressed above.
Then there is the mentioned problem with the use of useAsObservableSource outside of observer. Should we forbid it/warn (it would warn even in cases where react wouldn't...)?
And then I have a question about the concurrency (I really don't know how these fibers work):
Could it be that one fiber starts rendering, but the mutation notifies a reaction that belongs to a component of a different fiber, so the reaction would see there is a rendering in progress even though it's own fiber is not rendering at that moment? Does it makes sense, is it something to worry about?

[olee]

It may break some assumptions in the render function

As I said before, that should not be possible.
Right now react itself schedules the forceUpdated component to render sometime after the current one while printing the error (ie. throws an error but still works).
The only difference is that we schedule the forceUpdate into the phase of react where it itself would rerender the component when forceUpdated incorrectly as in the current code.
That's why the rendering process itself should be pretty much exactly the same as before.

Also you need useLayoutEffect

Already using it

Then there is the mentioned problem with the use of useAsObservableSource outside of observer

I improved the PR to separate the delaying logic of force-update calls into a separate hook which can then be used in those places.
Also I think it would be useful in general to be able to access this utility hook externally so I added it to the exports.

Should we forbid it/warn (it would warn even in cases where react wouldn't...)?

I think react already does this so this might not be necessary anymore.

And then I have a question about the concurrency (I really don't know how these fibers work):
Could it be that one fiber starts rendering, but the mutation notifies a reaction that belongs to a component of a different fiber, so the reaction would see there is a rendering in progress even though it's own fiber is not rendering at that moment? Does it makes sense, is it something to worry about?

I don't really understand your example - if it is still unclear for you, can you try to explain it again in other words?

[nulladdict]

Just a random thought I had, that might be relevant to the overall discussion.
So, initial problem is mixing observable and non-observable state, which results in tricky stuff we're dealing with right now.
Maybe, we should switch the approach from using non-observable state in computed variables and instead use observable state right in the render.

To illustrate, instead of this:

const Component = observer(props => {
  const state = useLocalStore(source => ({ foo: 1 + source.bar }), props)
  /* ... */
})

We can encourage something like this:

const Component = observer(props => {
  const state = useLocalStore(() => ({ foo: 1 }))
  const foo = state.foo + props.bar
  /* ... */
})

This way whatever uses non-observable state will be forced to compute synchronously right inside render, and we also might keep observable updates synchronous, unless there's outright mutation happening in render.
It certainly feels less cool and more verbose, but it's easier to reason about. If the computation is expensive, we can manually wrap it in useMemo

[urugator]

As I said before, that should not be possible.

I was referring to that HOC solution, where the Object.assign is inside the effect, so the observableProps are not immediately synced.

I think react already does this so this might not be necessary anymore.

Yea but it's not really obvious whats wrong, I quess we should at least mention it in docs.

can you try to explain it again in other words?

Gave it some more thoughts, it's probably fine.

[olee]

@urugator So afaik there are no more open questions.
Do you think we can then merge the PR?

[mweststrate]

Fixed in 2.1.0

[rizrmd]

I still experienced this bug using nextjs and mobx-react-lite 2.2.0, maybe I did something wrong ?

[danielkcz]

@rizkyramadhan Please open a new issue with all details and ideally minimal reproduction.