Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

implement lazy initialization to useObservable #72

Closed
wants to merge 3 commits into from

Conversation

alsotang
Copy link
Contributor

@alsotang alsotang commented Mar 3, 2019

Finally I solved the problem of typescript inference.
The code is simple. Just accept a initial function.

@coveralls
Copy link

coveralls commented Mar 3, 2019

Coverage Status

Coverage increased (+0.02%) to 98.995% when pulling 11e2e52 on alsotang:use-observable-lazy into acc0543 on mobxjs:master.

Copy link
Collaborator

@danielkcz danielkcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not too excited about this. I am still kinda convinced it opens up to bad practices. The heavy state should not be in the component like this as it gets lost on unmount. The Context is a much better candidate imo as you usually don't want to be prop drilling it.

The prop drilling is a problem especially because you cannot simply pass down a single prop from that observable as it would not be observable anymore. This is very different from useState which allows it. People might get burned by it a lot.

Honestly, there are so few people asking for this that I am not too keen merging it right away, but rather wait. I would like to see some real uses where having initializer is necessary. Personally, I had no need for it so far just yet so I am curious what are people doing that they need it.

src/useObservable.ts Outdated Show resolved Hide resolved
const TestComponent = () => {
const obs = useObservable(() => ({
x: 1,
y: 2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a getter and action here just to be safe on the type side. I tried what inferrence problem there is without overloading and it mostly works except that getter in the above test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added tests for action and getter in new commit.

@danielkcz danielkcz added enhancement New feature or request on hold No need to solve this right now discussion labels Mar 3, 2019
@alsotang
Copy link
Contributor Author

alsotang commented Mar 4, 2019

I don't know how to respond to your use-scene doubt.
I just think that useObservable and useState are both state initialize hooks. If useState provide a lazy way, useObservable should provide too.

In my opinion, useState is also designed for simple usage. If you ask me, is there any strong reason for useState to offer this way? Is there a better alternative? I can't respond either.

https://reactjs.org/docs/hooks-reference.html#lazy-initial-state

And I don't think this mr is much relevant to prop drilling.

I respect your opinion. You can decide if this mr is necessary. If you need it in the future, at least I provide a way to solve the type infer problem.

@danielkcz
Copy link
Collaborator

The useState is more often used for a single value state. Sure, objects work too, but you lose flexibility as you always need to merge with a previous state. On the contrary, the useObservable works for objects only, but setting a single value is easy.

The second huge difference is that if you want to pass observable state down, you have to grab a whole thing. Sure, you can pass down a single value, but it won't be observable anymore. With Context approach, you don't need to think about it because you always have a whole state which you don't need to split to smaller pieces. It's easier to track down its use.

That leads me to the point that useObservable was never meant for something heavy. It's just an escape hatch to have an easy observable state within a component that does not go anywhere further. I am even having doubts if it should have been in the lib at all. It was mostly a spur of the moment because Hooks were so cool so why not to have that.

If we openly support the possibility to have a heavy observable state, it might lead to bad patterns where you have some big bulk of data where it's not supposed to be. Consider, that nothing like that was possible with mobx-react. I don't even recall seeing a class component with some heavy observable state included. It was always about Context and people were fine with it.

Ultimately, I still want to wait a bit for more people asking for such a feature. If you know some other users of this lib, you can direct them here for their opinion. Or make some unbiased poll on twitter or something.

Also, I would like to see proper documentation that warns people about this kind of misuse which is really hard to debug without deep knowledge of MobX. Are you willing to take responsibility for answering questions of users who get entangled in this problem?

PS: Would you care to share your actual use case? Note that observable class object with 3 or 5 properties can hardly be considered heavy :) That's just premature optimization without measurement imo.

@alsotang
Copy link
Contributor Author

alsotang commented Mar 4, 2019

If a user would pass an observable prop down, and find dependency track not work. It's not due to a complex observable. It is because he doesn't understand the mechanism of mobx-react-lite. As long as the user uses an observable. This thing would happen. Why user just passes a prop of a complex object down but not a simple one?

I don't have any actual use case. I just wanna align to useState.

We can wait for more comments about this : )

@danielkcz
Copy link
Collaborator

danielkcz commented Mar 4, 2019

I don't have any actual use case. I just wanna align to useState.

We are already getting a single mutable object instead of a tuple [value, setter]. That's something that will never align :) So trying to do at least partial alignment feels strange :) Yea, let's wait.

Why user just passes a prop of a complex object down but not a simple one?

Sure, it's just my assumption based on that it's unlikely to have a use for a complex state within a single component. With a simple state, it's less likely to get burned by that imo.

@mweststrate
Copy link
Member

mweststrate commented Mar 22, 2019

I think I have a pretty solid use case in general: ViewModels, for example:

type Circle {
   x: number, y: number, radius: number 
}

const CircleEditor = ({ circle }: { circle: Circle }) => {
   const viewModel = useObservable(() => ({
           isSelected: false,
           dragVector: [ circle.x + 100, circle.y + 100 ],
           get color() { this.isSelected ? "red" : "blue" }
           // bunch of event handlers / methods
       },
       [circle] // we want to create a new view model if a different circle is passed in
   )

   useObserver(() => /* render stuff based on circle and circleVieModel */)
}

I think lazy initialization has 2 benefits here:

  1. 'cheap', for example dragVector isn't computed and constructed in vain during each render
  2. Explicit control over when a new view model needs to be created by using dependencies

On a contra argument, one could very well argue that this is a non-mobx specific pattern, and a generic hooks would be better here, which would make it look like: const viewModel = useLocalSingleton(() => observable({ /* the object */ }), [circle]), based on what @urugator proposed here: #74 (comment)

@danielkcz
Copy link
Collaborator

danielkcz commented Mar 22, 2019

Well, not that I would be convinced that adding two numbers together is so slow it needs to be lazy, but dependencies is a good thing. However, you have a problem here. What happens to a previous state? I don't know all the rationale for that example, but let's say isSelected is toggled and then a new circle is passed in. Is it supposed to be unselected again?

The way it is declared, it would make sense to do the reset, but I am sure there are cases where losing the previous state could be problematic. One might argue that for a state you want to keep you would just declare a separate observable which would work, but it seems odd to me.

I think we should keep useObservable simple as it, but we can introduce useStore that we discussed in some other issue here for advanced cases like this. It just needs to be carefully considered toward that previous state. That factory function could accept the previous state, but it would be null for the initial call, so that's weird also.

@danielkcz
Copy link
Collaborator

danielkcz commented Mar 22, 2019

Also, consider we have useComputed which already is lazily initialized and has dependencies. It would work for that dragVector just fine or it can even return an object with multiple properties related to the circle. However, there is a long-standing problem (#38) which I am still unsure of how to solve properly.

We shouldn't probably provide too much utility hooks that feel very similar. It only adds to confusion which one to use.

@urugator
Copy link

Imho there shouldn't be useObservable at all or it should also accept an initializer fn.
It's a way to decouple state initialization from the component definition and to avoid the recreation of objects at every render:

function createPoint(x = 0, y = 0) {
  return { x, y }  
}

function Point00() {
  const point = useObservable(createPoint);
}

function Point11() {
  const point = useObservable(() => createPoint(1,1))
}

Even with context, the "heavy" state still needs to be initialized somewhere:

function App() {
  const appState = useObservable(createAppState);
  
  return <Provider value={appState}>/* ... */</Provider>
}

We are already getting a single mutable object instead of a tuple [value, setter]. That's something that will never align

Actually I've been thinking whether the value should't be always boxed
const [map, setMap] = useObservable(() => new Map())

That factory function could accept the previous state, but it would be null for the initial call, so that's weird also.

IIRC the initial null value is one of the reasons why getDerivedStateFromProps does not have an access to previous props/state.

Not a fan of the dependencies/memoization idea.

@danielkcz
Copy link
Collaborator

danielkcz commented Mar 22, 2019

@urugator I have a feeling we are walking in circles in here. It's still about theoretical and contrived examples of "heavy" state but do you have some real example for that?

I am repeating myself, but if someone really wants to be hunting every millisecond of the performance, it's always possible to simply not use this helper at all. It shouldn't be a silver bullet for every use case. With mobx-react nothing similar was available and nobody complained.

function App() {
  // the App won't probably render anytime soon, why to bother with lazy init?
  const appState = React.useRef(createAppState()).current;
  
  return <Provider value={appState}>/* ... */</Provider>
}

@urugator
Copy link

urugator commented Mar 22, 2019

I don't see what seems to be so contrived/theoretical. The use case is exactly the same as for useState, the callback basically replaces the constructor of class based component.
It's not a "lazy init", it's "single time init" ... the state is initialized lazily in both cases (during first render).
It's not a silver bullet, the use case is simple and focused - provide local (observable) state. It's initialization is part of the problem.
You can think about it the other way around: The ability to provide a value directly instead of factory is just a convinience for trivial state initialization.

// the App won't probably render anytime soon, why to bother with lazy init?

Because it's less bothering than making assumptions about the surroundings (how often the component re-render or how "heavy" the state actually is) and dealing with ref/useState workarounds.

@danielkcz
Copy link
Collaborator

I don't see what seems to be so contrived/theoretical. The use case is exactly the same as for useState, the callback basically replaces the constructor of class based component.

I don't think it's right to compare it to useState really. The mutable nature of observable is fairly different and without some observer in place, it won't even work. People should be aware of such a different concept and adopting the same API would only make that worse in my opinion. It shouldn't be seen as an equivalent solution.

In overall I think I would rather provide some recipes/faq for tackling complex scenarios and rather promote DIY approach than creating a super universal (and complicated) API. It only makes adoption harder when a newcomer gets overwhelmed by a bunch of similar options. Simple and straightforward is usually a better option imo.

People should learn that MobX is not some magical land that needs a bunch of utilities before it can be used. It's a similar case with Provider/inject. They got used for such hand-holding, that even the idea of using Context without any abstraction feels weird to some of them.

In practice, I've used useObservable very rarely, because I have models sitting in Context. Often the useState has worked much better as it did not need any observer to make that work.

@urugator
Copy link

urugator commented Mar 22, 2019

To me these seem to be arguments against useObservable rather than init function.

@danielkcz
Copy link
Collaborator

To me these seem to be arguments against useObservable rather than init function.

Sure, that can be as well. Along with a fresh discovery that useDisposable is fairly useless and useComputed is weird we can probably strip down a whole library to basics only and be happy again.

I am even having doubts if it should have been in the lib at all. It was mostly a spur of the moment because Hooks were so cool so why not to have that.

I've said that above ... #72 (comment)

@joenoon
Copy link

joenoon commented Mar 23, 2019

I have a use-case where useObservable(fn) would probably make sense. I've worked around re-creation using useMemo and observable directly, although that seems imperfect for other reasons. I'm not sure this is the solution I'll keep... maybe I'll come up with a better way. I implemented this right when this repo was getting started, so the solution I came up with might be stale. But for now, I have a component that does an Apollo useQuery, turns the result data into an observable (memoized), then uses that observable in a form. The data is a decent size and a few levels deep and the form is fairly involved.

I'm catching up on this thread and seeing maybe there are ways to avoid useObservable all together which I'll look into.

@danielkcz
Copy link
Collaborator

Ok, that's certainly an interesting approach. The useMemo is definitely a bad candidate as in the future React may decide to throw it away randomly and recreate. If you look at the implementation of useObservable from this PR it's fairly simple with useRef

The tricky part is what do you do when query decides to update? I mean useQuery is kinda observable on its own and if for whatever reasons the cached data changes, the query will provide new data. It would make sense to merge that in, but only that user hasn't changed yet 🙈

Anyway, it's getting more to the point that one universal useObservable will never suffice for every use case and it's fairly opinionated. Instead, we are going to provide better documentation, FAQ or recipes on how to handle these common scenarios.

@danielkcz
Copy link
Collaborator

danielkcz commented Mar 24, 2019

I am going to close this since we are considering removal. It might still come in handy if we ever decide to make a separate utility package.

#94

@danielkcz danielkcz closed this Mar 24, 2019
@alsotang
Copy link
Contributor Author

ok

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion enhancement New feature or request on hold No need to solve this right now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants