Using props in class-based stores

[danielkcz]

the issue is that (for the mechanical refactor to hooks) the class store needs access to an observable version of the props that autoupdates as new props arrive so it can have computed values (and/or actions) that depend both on the state and the props. AFAICT useLocalStore ensures that the props being received are a mutable mobx object being continuously updated, or am I mistaken?

Originally posted by @spion-h4 in mobxjs/mobx-react#722 (comment)

[danielkcz]

I don't think it's actually possible to access props with a class store or at least I don't know-how. The problem is that the class store doesn't share closure with the component.

What you can do is utilize useAsObservableSource to create an observable variant of props and pass that to a constructed class.

const Comp = ({ propA, propB }) => {
  const source = useAsObservableSource({ propA, propB })
  const [store] = React.useState(() => new ClazzStore(source))
}

class ClazzStore {
  constructor(source) {
    this.source = source
  }
  @computed
  get something() {
    return `${this.source.propA} / ${this.source.propB}`
  }
}

Fairly verbose if you ask me, but I don't think there is some other "magical" way. The useLocalStore basically does the same, but it's slightly easier. For a reference...

const Comp = ({ propA, propB }) => {
  const store = useLocalStore((source) => ({
    get something() {
      return `${source.propA} / ${source.propB}`
    }
  }, { propA, propB }) // useAsObservableSource is baked in
}

Frankly, I don't know why is everyone so hyped about class stores 😆

[spion-h4]

Because contrary to current popular belief, classes are a fine choice for this.

Maybe something like this?

interface BaseStore<T> {
  props:T;
}

function useClassStore<T, U extends BaseStore<T>>(Klass: {new(t:T):U}, props: T) {
  let source = useAsObservableSource(props);
  let [state] = useState(() => new Klass(source))
  return state;
}

I'm assuming that the contents of the source reference returned on the first render will be updated every time useAsObservableSource is called? With hooks, you can never tell what the exact semantics are...

[danielkcz]

Yea I suppose such hook will work. If you are asking about including such a hook to the library, I am don't think it's a good idea as it's very opinionated. At the moment you have a store that has a different constructor argument, you cannot use that. That can be important in case of dependencies on other stores. Besides, it's just 3 lines of code, no special handling or logic. Anybody who needs that can just copy that.

I'm assuming that the contents of the source reference returned on the first render will be updated every time useAsObservableSource is called? With hooks, you can never tell what the exact semantics are...

You are assuming right. With hooks, you can't tell much, but with MobX it's very important to have a stable reference so it's safe to assume you get that.

Because contrary to current popular belief, classes are a fine choice for this.

Would love to learn why. They are just more verbose in my eyes and require special handling due to decorators. I don't see any real benefit except it's defined "outside of the component". But you can as well have a custom hook for each store that utilizes useLocalStore and outcome is pretty much the same except its bit cleaner.

[urugator]

You can also simply update whats necessary every render:

const [store] = React.useState(() => new Store())
store.setDep1(props.dep1);
store.setDep2(props.dep2);

[danielkcz]

@urugator That sounds easier to you? :) We have MobX here and relying on imperative updates sounds fairly awkward. Besides, such operation is a side effect and as such should be in useEffect. That means it will be delayed. Suddenly, you have one value at component props and outdated value in the class instance.

[urugator]

@FredyC It works exactly the same as useAsObservableSource:

const [source] = React.useState(() => observable({}))
runInAction(() => {
   Object.assign(source, props);
});

It's impossible to get "reactive props" without imperative side effect (copying props to observables). We can do this because Mobx is synchronous, so once you call the setter (or Object.assign()) everything is up to date.
It is awkward, but no more than using useAsObservableSource or useLocalStore. Principle is really the same - copy props to a local observable and use it instead of props directly.
This is just a lot more transparent and easier to follow imo.
EDIT: note that you can store.setProps(props) similary to passing it to constructor, my point is that you don't need useAsObservableSource as it basically does the same thing, however I think the class preferably shouldn't know anything about props object

[danielkcz]

We can do this because Mobx is synchronous, so once you call the setter (or Object.assign()) everything is up to date.

Yea, but isn't also against "don't change state during a render"? Moreover that MobX is a synchronous and changing state in the middle of rendering sounds like something must go wrong. And yet it obviously works, at least now, but I wonder if that will still work in Concurrent mode 🤔

It is awkward, but no more than using useAsObservableSource or useLocalStore.

The awkwardness in my eyes comes from the fact that you must adapt your store (and do it for each one) to accept some outside values. In the case of useLocalStore you get the source as an argument and you build the store with it. It feels much more natural and straightforward. Sure, you can derive the class and have it there, but that comes with some price as well.

[urugator]

Yea, but isn't also against "don't change state during a render"

It's not kosher and hardly something to recommend.

In the case of useLocalStore

I am not advocating use of classes for local component state (and neither useLocalState).
In case of refactoring I would invest time into transforming local observable/computed/action into useState/useMemo/useCallback or moving it to (normalized) global app state (mobx class or whatever).
I don't see much of a point in refactoring to hooks, while making things more complicated (and actually still avoiding them).

[danielkcz]

I am using MobX local store merely for its convenience as I don't need to bother with listing deps in effects and callbacks. That's a major selling point for me. I am not sure what "complications" you are mentioning. On the contrary, things are complicated with immutable state.

[urugator]

don't need to bother with listing deps in effects and callbacks

You need to bother with listing deps to useLocalStore
How does local store help with effects?
Action depending on props is a bad idea - that should be another rule to follow.
Remains computed, which is actually rarely needed (in a sense that provided optimization is effective).

complicated with immutable state

I have no issues with useState(() => observable({})) or useState(() => new Store())
But these can't be used if they depend on props - you either need "hack" or hooks, so I am suggesting hooks...

[spion-h4]

The only reason I'm considering this (mechanical) refactor is because the "new" React is the only one getting new features (for example, there is no ability to access multiple contexts from the old react, while useContext can be invoked multiple times).

I'm absolutely fine with continuing to use classes otherwise; in addition, the new state management hooks provide significantly worse experience compared to MobX so I don't think they're an option.

For reference, the fully mechanical refactor is as follows:

Assuming the component MyComponent

• you extract the render to a function
• you do let thisState = useLocalState(props => new MyComponentState(props), props); at the top
• you replace this with thisState in the render method
• you rename the old MyComponent class into MyComponentState, remove extends React.Component from it and add a constructor(public props:MyComponentProps) {}

There is however the issue of double-rendering:

https://codesandbox.io/s/aged-snow-5u146

Try changing the props of the component by incrementing "s.a"; two renders will be triggered, one by react due to a change in props, another due to useObserver being triggered by that change

I'm not sure how to avoid that one. By the time we get new props, we're already in a render cycle, so we can't really avoid one. In fact even the React docs recognize that this is not possible using hooks: https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-shouldcomponentupdate so it seems a more elaborate wrapper will be necessary to get the old functionality.

[spion-h4]

@urugator the reason we use local observable and computed instead of local component state and "useMemo" is because they are an excellent fit. Typically components will receive a somewhat complex MobX model objects via props (for example a document item in a list of document items); however they need better refined / transformed data (a view if you will) suitable for presentation (defined via computeds, for example compute item summary string name from several item props) with additional stateful options of that particular view component (observables, e.g. whether the item details are expanded or not) for that view that can be modified (via actions like expandDetailedView triggered by a button)

Using observable and computed ensure that changes in the model will result with minimal and efficient recomputation of the view data; if the view consists of multiple pieces, it can also ensure that the smallest amount of DOM nodes re-render and update. useMemo and friends are absolutely inadequate and underpowered in comparison - they are not worth even considering.

[urugator]

issue of double-rendering:

Yes this is disadvantage of hooks, there is no way to bailout (I wonder whether we will use generators one day, yielding the effects instead of calling hooks).

useContext can be invoked multiple times

I used a HOC like: consume({ store1: Context1, store2: Context2 })(({ store1, store2 }) => {})
If the context value updates often (probably not with mobx, but it's common with immutable state), you may still want to put memo between useContext and actual component (or wrap render in useMemo).

useMemo and friends are absolutely inadequate and underpowered in comparison

Just keep in mind that reactive props is nothing but a state change inside derivation:

class PseudoComponent {
  @observable local; 
  @observable props;
  
  @computed a() {
    this.local + this.props.x;  
  }
  
  this.render = computed(props => {
    // changing state, breaking one-way data flow    
    runInAction(() => {
      this.props = props;
    })    
    return this.a;                       
  }, {
    equals: diffVdom,
  })
}

By moving props into constructor you will just move the problem one level up. Eventually you will realize that all deps must be provided upfront (which is kinda obvious considering computeds are argument-less), otherwise you need to memoize over arguments.
Memoization/diffing is perhaps slower (than binding), but allows you to have dynamic dependencies (deps unknown at the time of definition) and therefore dynamic component hierarchies.

[spion]

So far, hooks have been nothing but a disadvantage :) I have a feeling that they might end up being a huge failure: they are very easy to write but very hard to read (and especially to understand the semantics of what runs when) which seems to me like a recipe for disaster.

Would love to learn why. They are just more verbose in my eyes and require special handling due to decorators. I don't see any real benefit except it's defined "outside of the component". But you can as well have a custom hook for each store that utilizes useLocalStore and outcome is pretty much the same except its bit cleaner.

In TypeScript classes are the only construct that at the same time represent a type and a runtime object. This gives them many advantages, one of which is that they can enable easy, type-safe dependency injection by representing both the interface, the implementation and the DI token (e.g. you could supply them automatically via emitDecoratorMetadata by reading design:type or design:paramtypes which only work for classes)

They can also just replace interfaces + type-safe DI tokens without the implementation:

class ActuallyAnInterface {
  field1!: string;
  field2!: stirng;
  methodReturningString!: () => string;
}

The "easymode" classes are still the most important ones, because for most of them you don't need to separate the interface from the implementation until needed.

In our "framework", through react context, we access the injector, which provides the application singletons as well as component level transients: hfour/h4bff#57

You can read more about the framework here: https://hfour.github.io/h4bff/#/ although the building blocks and the docs on how they fit together are not fully complete yet.

[spion]

So I think I managed to avoid the double-updating issue by making the props mutable and turning off the staleness notification for the reaction temporarily while assigning the new props:

https://codesandbox.io/s/trying-to-use-hooks-794py

This is absolutely fine, since the reaction is already "scheduled to run" - we're re-rendering the entire component after all.

Not yet sure whether its a good idea to pass on the observable props directly - I think it might be better to keep react props object semantics but implement another hook called useObservableProps(props) that simply fetches the already created observable props; only available in a hObserver style component of course.

edit: props are now immutable; useObservableProps hook provides a MobX observable reference of them which is already created by hObserver

[spion]

@urugator & @FredyC I believe the above technique separates the "props receiving" phase from the "computing the VDOM" phase nicely. When the props change, mobx gets out of the way since the render is going to happen anyway and there is no need for reactions; in all other cases, mobx takes care of re-renders. If I had access to mobx internals I would describe this as "reaction is already scheduled to run" but without that bit disabling the event was my best idea.I believe this implementation mirrors the original mobx-react API and behavior more closely.

Do you think there is a place for useObservableProps in mobx-react(-lite)?

[danielkcz]

@spion I am kinda failing to follow it here and what exactly are you proposing. How is it different from useAsObservableSource? Can we adapt that existing hook?

[spion]

Its different in that the component wrapper itself runs the hook to create an observable version of the props, and the useObservableProps only provides the mutable object that was already created by the wrapper. So you cannot use a different observable source.

What it does give you, however (that useLocalState doesn't) is the ability to prevent double-rendering. With useLocalState double rendering happens every time there is an update to the contents of the argument passed to it.

It has to be this way because the reaction needs to be disabled during the props update, and the props update needs to happen outside of the tracking region of the reaction otherwise disabling doesn't work

I tried implementing useAsObservableSource that temporarily disables the reaction, however if that's done during the tracking stage it messes up the state of the reaction:

https://codesandbox.io/s/trying-to-use-hooks-n469q

Click on a+=1 to update the props of the inner component, then on counter += 1. The second click will not trigger a render of Add. If you do the two steps again, counter+=1 will trigger a re-render the second time. Not sure why this happens (probably because its lowestNewObservingDerivationState gets messed up during tracking so onBecomeStale doesn't get called next time)

[danielkcz]

Can you draft how would the useObservableProps looks like? I suppose the one from the sandbox, is not it? In case it is then I am sure we don't want anything depending on global variables with Concurrent mode coming.

let currentProps: any = null;

function useObservableProps<T>(_props?: T): T {
  if (currentProps != null) return currentProps;
  else throw new Error('useObservableProps not available for non-observer component');
}

And btw, the observer got changed in mobx-react-lite@2 (published under @next) to account for Concurrent mode, so you might want to experiment with that first.

---

I seem to have lost what is all the reason for it. Some kind of micro-optimization to avoid one extra re-render? I thought we were talking about props in class components in here :)

[spion]

Its not a micro-optimization if every single prop update causes two calls to the render function, is it?

I'm trying to write a hook version that gets similar behavior to the class based implementation of mobx-react; that includes:

• support for class based state
• support for fully observable props
• single render when props update

[spion]

@FredyC The global variable cannot be in an incorrect state, since its not possible for concurrent react to interrupt the section where its available or run it in parallel with other sections:

currentProps = props; // this is where the global variable becomes available
    reaction.current.track(() => {
      try {
        rendering = fn(p);
      } catch (e) {
        exception = e;
      }
    });
    currentProps = null; // this is where its no longer available

Concurrent react can only postpone the full rendering of child components, it cannot interrupt a standard function call.

BTW thanks for pointing me to the updated code - I'll mess around with it and see if I can get it to do what I need :)

[danielkcz]

Its not a micro-optimization if every single prop update causes two calls to the render function, is it?

You know, I am fairly convinced that with MobX it doesn't happen that often to have props changing like crazy. Usually, you would be passing some stable store there. Sure, couple props might be needed to change from time to time, but imo it's not that often as it might seem. And if a component is really that convoluted to have several sources of re-render, it might be worth to rethink that and split up.

Concurrent react can only postpone the full rendering of child components, it cannot interrupt a standard function call.

Doesn't it also mean it can render several components at a time? I haven't really examined it that much, but "concurrent" sounds like "parallel" to me.

I think that automatically wrapping all props to observable might be overkill. It would be done no matter if you actually use those props. As such I don't think it should be the default behavior, but opt-in.

There is a hidden limitation. The useAsObservableSource needs a stable shape object. That means it would crash in case pass slightly different composition of props to an observed component. That's another argument why it shouldn't be the default behavior.

Also, I am kinda confused. In useObservableProps you are accepting props, but not doing anything with that. I guess some experimental leftover?

The name useObservableProps is slightly misleading as it doesn't use any hooks and as such can be called from anywhere (except it will fail with an error when done that).

Lastly, it feels somewhat opinionated. Personally, I am using useAsObservableSource not for props only, there are also values from context or even "calculated values" based on props. With your solution, I would gain some performance but lose that benefit.

I am not saying it cannot become part of mobx-react-lite, but it certainly needs a good test coverage to figure out pitfalls.

[spion]

Also, I am kinda confused. In useObservableProps you are accepting props, but not doing anything with that. I guess some experimental leftover?

Yeah that part is suboptimal :( The issue is you need to either pass the props or the type of the props - otherwise typescript wont know what type of object to return.

Ordering rules don't apply for this "non-hook" so its not quite a hook, agreed. I thought it would be less confusing if it was presented as a hook

My first version was nicer because it simply presented a mutable, observable props object as the argument so there was no need of useObservableProps. Its quite clean, but potentially a huge change of behavior if the props object is passed as a whole anywhere, and it violates the "frozen-in-time snapshot" concept that hooks try to present.

I don't necessarily expect this to become a part of mobx-react or mobx-react-lite as it is - I wanted to bounce off a few ideas about observable props and class based stores and see if I can get help coming up with some API that approximates the class API but also makes sense in a hooks context. Its not easy!

To be honest I am surprised to hear that people aren't using mobx state in class components - was everyone using setState instead? Seems like such a natural extension for UI state like e.g. filter toggles, dropdown expansion, currently selected items etc... is everyone keeping that state outside the components too?

[danielkcz]

To be honest I am surprised to hear that people aren't using mobx state in class components - was everyone using setState instead? Seems like such a natural extension for UI state like e.g. filter toggles, dropdown expansion, currently selected items etc... is everyone keeping that state outside the components too?

The setState isn't inherently bad for simple stuff really. The class components are a bigger burden to treat that state properly imo. Duplicating code, no easy way to share common stuff, forgetting to clean up and a bunch of others.

Personally, I still sometimes opt for useState, mostly in cases where observer just feels like overkill. In conjunction with Immer, the useState becomes even easier for more robust stuff. The useState becomes annoying in a moment you need various setters to be passed around. Sure, there is a useReducer ... but no thanks :) With MobX this isn't a problem as the useLocalStore can carry actions included in a store. Or even class-based stores are fine in that matter.

I never liked decorators in classes and in the essence classes alone. It just did not feel right. So I was rather using recompose like crazy with 10+ compositions over the functional component. It was hell to debug that, but it certainly helped me to mentally shift to hooks faster because I wasn't used to classes anyway :)

[danielkcz]

I think we have exhausted the topic here with loads of useful information. For further questions, please open a new issue.