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

Get rid of (almost) all utility hooks #94

Closed
danielkcz opened this issue Mar 22, 2019 · 121 comments
Closed

Get rid of (almost) all utility hooks #94

danielkcz opened this issue Mar 22, 2019 · 121 comments
Labels
discussion help wanted Extra attention is needed

Comments

@danielkcz
Copy link
Collaborator

danielkcz commented Mar 22, 2019

Update

After a lengthy discussion, we have agreed on the removal of useComputed and useDisposable. The useObservable hook will be renamed to useAsObservableSource and meant primarily for turning props/state data into observable. These can be then safely used in a new hook useLocalStore which will support lazy-init and serve as the main way of constructing observables within a component.

Check out a comment with an initial proposal: #94 (comment)


Time has come to start considering this. There have been some discussions lately that got me convinced these utilities should not have been in a package in first place. I think we got carried away in here, most likely because it felt good to have some custom hook 😎 Ultimately, people might get the wrong idea that to use MobX in React they need to use these specific hooks.

I think it's better to do this sooner than later before people start using these utilities too much and would need to painfully migrate later.

As the first step, I would like to focus on preparing a website dedicated to MobX in React that would show several recipes and DIY solutions. I would like to hear some recommendations on what toolkit would be the best candidate for such a site. Gatsby?

The idea of some separate package with utility hooks is not eliminated, but it should go hand to hand with new docs to thoroughly explain the concepts and not to just blindly use the hook without fully understanding implications.

useComputed

The most controversial and not even working properly. After the initial pitfalls, I haven't used this anywhere. Is someone using it successfully?

useDisposable

As @mweststrate discovered today, there is not much of the benefit to this as the React.useEffect does exactly the same. The only difference is access to early disposal function. Personally, I haven't used for anything just yet. I would love to hear use cases if there are any. I am almost ashamed I haven't realized this before and just blindly used it 😊

useObservable

Probably the most useful out of these three and the most discussed (#72, #7, #22, #69) also. It's clear that it's not only confusing but in its current form, it's wrong altogether. Personally, I have rather used React.useState for a component local state which is so easy and doesn't require any observer. There is not much performance gain anyway unless <Observer /> is used. For the shareable state, it seems better to just use Context and build such a state in a way people like. Also, it makes a little sense to be resetting the whole MobX state based on props change.

@xaviergonz
Copy link
Contributor

useComputed: never saw much use for it to begin with tbh

useDisposable:

the problem I see with useEffect (besides not returning the early disposer) is that without some guideline it can be easily misused, e.g (with reaction):

  • forget to pass the deps array and it will be created and destroyed on every re-render
  • pass the deps array with the actual observables being used to react to and it will basically run in a weird way (unless fireImmediately is used) and be unnecessarily recreated
  • use fireImmediately and don't use useLayoutEffect and it will react after the first time the component is painted, causing an unneeded re-render
  • maybe some more I'm forgetting :)

so I think it is still good to keep it there just for the sake of "what's the best way to use a mobx reaction/autorun etc within hooks?"

but that's the point again, the user shouldn't have to worry about it.

useObservable:
I think it could be just be a useReference, so maybe that one could be a goner

@xaviergonz
Copy link
Contributor

xaviergonz commented Mar 22, 2019

On a side note, I wonder why react doesn't offer a useReference with a lazy initializer
That's actually what I'd use as useObservable, giving it a new class (but probably I'd name it something different like useLazyRef)

or well, I just guess useState is good enough (even though it will re-render twice on mount)

edit: actually it doesn't rerender twice, just checked, that's nice

@danielkcz
Copy link
Collaborator Author

the problem I see with useEffect (besides not returning the early disposer) is that without some guideline it can be easily misused, e.g (with reaction):

Well, the useEffect can be easily misused even without MobX :) It's definitely the biggest gotcha in React Hooks, but with the help of articles like The One it's getting to be more understood. Note that current useDisposable does not protect you from those mistakes because it allows you to pass deps anyway.

It's great you listed those gotchas, haven't even thought about those. Would be a great source when explaining them in the FAQ.

so I think it is still good to keep it there just for the sake of "what's the best way to use a mobx reaction/autorun etc within hooks?"

If anything, there could be a separate package with such specific utilities as useReaction or useAutorun, but the primary focus should be on documenting and explaining that those are not needed for MobX in React.

On a side note, I wonder why react doesn't offer a useReference with a lazy initializer

I am sure I've seen some explanation from Dan somewhere, but cannot seem to find it 😖

@xaviergonz
Copy link
Contributor

xaviergonz commented Mar 22, 2019

Note that current useDisposable does not protect you from those mistakes because it allows you to pass deps anyway.

Well, actually deps are still required (sadly) in case you (for example) need to use a state or prop inside the effect code, or else they will be stale :-/ (and that will require the reaction to be recreated just because of the weird way react handles deps, which sucks)

(makes me wonder if the eslint rule that warns about missing stuff in the deps array of useEffect would also warn about other hooks, if so then that would certainly be a big reason -not- to have it as a separate hook, yet have a guideline somewhere)

@xaviergonz
Copy link
Contributor

Somehow I have the feeling react and mobx in the end want to do the same thing but in totally opposite directions. It feels like "do it the mobx way or the react way, but if you try to mix them it will be harder" (but I guess that also used to apply with class components and setState)

@xaviergonz

This comment has been minimized.

@danielkcz
Copy link
Collaborator Author

danielkcz commented Mar 22, 2019

As much as nice this looks at first, the real component could get ugly really quickly in my opinion. There is too much cognitive overhead entrusted into the component that should be mainly about rendering stuff. It also creates a shadowy assumption that you need all these hooks to have MobX in React and that's something I want to avoid for sure.

Nah, any complex state should be constructed out of the component, ideally in the Context. For a simple local state, the React is good on its own. I wouldn't call it silly for sure. It's just different.

const Component = memo(function (props) {
  const obsProps = useMobxProps(props);
  const state = useMobxState(() => {
    x: 5
  });
  const views = useMobxViews(() => {
      get xPlus1() {}, // computeds
      viewFunc(foobar) // non action functions
  });
  const actions = useMobxActions(() => {
    // setX
  });
  useMobxEffects(() => [
    reaction(() => state.x === obsProps.x, () => {...});
  ]);

  return useObserver(() => { ... })
})

@xaviergonz

This comment has been minimized.

@danielkcz

This comment has been minimized.

@xaviergonz

This comment has been minimized.

@JabX
Copy link

JabX commented Mar 25, 2019

Nah, any complex state should be constructed out of the component, ideally in the Context. For a simple local state, the React is good on its own. I wouldn't call it silly for sure. It's just different.

I have a ton of components that use complex state effectively, with a lot of computed values to minimize rendering. Not all components are simple, and we should provide ways to enable people to do their stuff, as long as they're not trivial or redundant.

Using useRef for anything else than an actual ref is cumbersome at best, so I'm all for having useful utilities that wrap it. useObservable fills a need that you can't replicate with any other solution (well, I guess you could stick with a class component), and I'd even argue that its API should be complete, with "lazy" initialisation and especially with decorator support (the second parameter). And no, useState doesn't cover the same usages at all. useReducer exists in the standard API and that's what it replaces. I am totally fine with building the entire state of all my components with one call to useObservable in each and keep working like I've been for the past couple of years with local observable state.

useComputed is maybe a bit too simple and you can do the same thing with useObservable and a getter. I have no problem with removing it, but I'd probably use it here and there if it stays.

useDisposable(() => autorun(() => …)) is the same as useEffect(() => autorun(() => …), []) (well, except for the part where it doesn't return the disposer). While they both look like each other, the empty array is key (especially when using autorun that runs immediately) and for me it's a big no no, especially since not specifying explicit dependencies is (one of) the main draw of using MobX, so I don't want to think about it ever. Since useDisposable in itself doesn't simplify that much the API, I'd be more in favor of having distinct useAutorun, useReaction hooks that have the exact same API than their regular counterpart, with the added benefit of being disposed at unmount (ie without double functions). Also, that would mean that a useAutorun would run during (before?) the first render, as it should (like with @disposeOnUnmount).

But for me the real redundant hook is the useObserver one, especially since observer is more predictable (see #97) and Observer is more flexible (you can have several independent reactions that don't trigger the main render). If the only advantage of using useObserver is having a cleaner React tree, it's not worth it, especially since forward refs solve most of the issues of doing this like that. I get that useObserver is the "original" API from which everything else is based, but it could simply stay as internal.

@danielkcz
Copy link
Collaborator Author

danielkcz commented Mar 26, 2019

I have a ton of components that use complex state effectively, with a lot of computed values to minimize rendering. Not all components are simple, and we should provide ways to enable people to do their stuff, as long as they're not trivial or redundant.

I am curious how do you minimize rendering with useObservable? Do you realize that even the useObserver will rerun a whole component on change because it uses useState underneath? The only way to minimize rendering within bounds of a single component is use of the <Observer> in the rendered tree.

Using useRef for anything else than an actual ref is cumbersome at best, so I'm all for having useful utilities that wrap it. useObservable fills a need that you can't replicate with any other solution (well, I guess you could stick with a class component), and I'd even argue that its API should be complete, with "lazy" initialisation and especially with decorator support (the second parameter).

Sure, I am not against the idea of having a separate package with such utilities. Could be even a multiple hooks for simple cases and for the complex ones. The core idea is to have a mobx-react package which can be used purely for observing components. Anything extra (including how to create observable) should be aside because it will always be opinionated.

But for me the real redundant hook is the useObserver one, especially since observer is more predictable

I guess it's personal preference, but useObserver feels more apparent. When there is HOC around the component and tend to miss it. And there is also automatic memo for the observer which burned me a couple of times already. Lastly, the observer (and useObserver) cannot see anything in render prop pattern which is rather misleading and source of ugly bugs.

While they both look like each other, the empty array is key (especially when using autorun that runs immediately)

I don't think it's such a big deal. If you forget to use [] in the useEffect call, it only hurts a performance slightly. Later you learn about it and use it. Also sometimes the autorun or reaction depend on non-observable variable and in that case you should specify dependency. I am convinced it's better to be explicit than trying to be clever.

well, except for the part where it doesn't return the disposer

Do you possibly have some use case for it? It's the biggest part of that code which doesn't seem that useful really.

@danielkcz
Copy link
Collaborator Author

danielkcz commented Mar 26, 2019

Either way, as I said above, I really want to start with preparing some documentation site before touching any code. It's a long run. Still waiting for some recommendation on what to use? I am no designer, so I would prefer something with a useful template that can be tweaked later. I looked at Gatsby templates and doesn't seem like a good choice for docs.

@JabX
Copy link

JabX commented Mar 26, 2019

I am curious how do you minimize rendering with useObservable? Do you realize that even the useObserver will rerun a whole component on change because it uses useState underneath? The only way to minimize rendering within bounds of a single component is use of the <Observer> in the rendered tree.

Well, not directly with observables, rather with computed values and actions declared inside. I mean, that doesn't change what we've been doing for quite a while with class components and decorators (which is why I also want decorator support)

Sure, I am not against the idea of having a separate package with such utilities. Could be even a multiple hooks for simple cases and for the complex ones. The core idea is to have a mobx-react package which can be used purely for observing components. Anything extra (including how to create observable) should be aside because it will always be opinionated.

They're not complex, very useful and not at all opinionated. Not to mention that they don't take a lot of space inside the package. Not providing them in the base package will confuse people because they cover basic usages that are already built in for classes. If anything, we should aim for parity between the two if we want people to switch (I know I want that, at least)

I guess it's personal preference, but useObserver feels more apparent. When there is HOC around the component and tend to miss it. And there is also automatic memo for the observer which burned me a couple of times already. Lastly, the observer (and useObserver) cannot see anything in render prop pattern which is rather misleading and source of ugly bugs.

Well, observer has worked with a HoC-like API for as long as I've been using MobX and it's been fine ever since. I don't see the point of introducing a new API that will only confuse people into which one they need to choose. Especially if it's less capable than the original.

I don't think it's such a big deal. If you forget to use [] in the useEffect call, it only hurts a performance slightly. Later you learn about it and use it. Also sometimes the autorun or reaction depend on non-observable variable and in that case you should specify dependency. I am convinced it's better to be explicit than trying to be clever.

I have never written a reaction that has a dependency on a non observable prop, how would it even work? Omitting the empty array is a big deal if using autorun because your reaction will run after each render, in addition to the places it should already run.

well, except for the part where it doesn't return the disposer

Do you possibly have some use case for it? It's the biggest part of that code which doesn't seem that useful really.

No, I was just stating that fact for the sake of being exact. I don't think there is a valid use case for an early disposal, especially if we can use useEffect to do it.

@danielkcz
Copy link
Collaborator Author

They're not complex, very useful and not at all opinionated.

Since we are discussing if there should be lazy-init and/or dependencies, it is opinionated as long as we cannot agree on the same variant. It shouldn't either exist at all or it should be different hooks. I don't like the idea of mixing everything into one bulk with overloads. It should be clear from the code at first glance how is that observable constructed and if it depends on something. Having different names for such utils usually serves the best.

Not providing them in the base package will confuse people because they cover basic usages that are already built in for classes.

Note that current mobx-react does not provide any sort of utility like that and I am not aware every of some complaints about it. There is the mobx-state-tree for advanced cases (my preference) or the form with class & decorators. I've already seen several questions comparing these two. Adding the third one into the mix makes it even worse in my opinion.

if we want people to switch

Um, switch to what? Soon the mobx-react V6 comes out and this package becomes pretty much obsolete unless someone really wants to avoid dragging class component support along.

Well, observer has worked with a HoC-like API for as long as I've been using MobX and it's been fine ever since. I don't see the point of introducing a new API that will only confuse people into which one they need to choose. Especially if it's less capable than the original.

Less capable how? It's a low-level API sure, but it has exactly the same capabilities and it's slightly more verbose and apparent from the code. The HOC tends to be rather hidden.

I have never written a reaction that has a dependency on a non observable prop, how would it even work?

Consider for example you need a conditional reaction that depends on a prop value. Since you cannot just call the hook conditionally, you have to put that condition inside the reaction. And that means the reaction needs to be disposed and recreated when dependant prop changes. Otherwise the reaction would be always seeing closure value when it was created first time.

@xaviergonz
Copy link
Contributor

@JabX you might want to check out https://github.com/xaviergonz/mobx-react-component
I just created it so I'm open for ideas

@mweststrate
Copy link
Member

mweststrate commented Mar 27, 2019 via email

@JabX
Copy link

JabX commented Mar 27, 2019

They're not complex, very useful and not at all opinionated.

Since we are discussing if there should be lazy-init a/or dependencies, it is opinionated as long as we cannot agree on the same variant. It shouldn't either exist at all or it should be different hooks. I don't like the idea of mixing everything into one bulk with overloads. It should be clear from the code at first glance how is that observable constructed and if it depends on something. Having different names for such utils usually serves the best.

IMO the hooks should have the same API as the original function (observable, autorun, reaction…), and as a hook useObservable should allow for "lazy" initialisation. That's all, and that's what I meant by saying "unopinionated": this is a 1-to-1 translation.

Not providing them in the base package will confuse people because they cover basic usages that are already built in for classes.

Note that current mobx-react does not provide any sort of utility like that and I am not aware every of some complaints about it. There is the mobx-state-tree for advanced cases (my preference) or the form with class & decorators. I've already seen several questions comparing these two. Adding the third one into the mix makes it even worse in my opinion.

Well, I can use @observable and @computed in class components. Sure, that's not a mobx-react thing, but these are the most basic things I use MobX for in my components today. And since hook-based components can be stateful, then we need something that allows us to declare MobX state in that. useObservable can do everything (as long as it mirrors the observable API), its API is already known (though I suspect not used all that much) and it's a good fit, especially considering that React itself provides useReducer for the same usage.

if we want people to switch

Um, switch to what? Soon the mobx-react V6 comes out and this package becomes pretty much obsolete unless someone really wants to avoid dragging class component support along.

Switch to using hooks in general. There are a lot of places in my codebase where hooks make a lot of sense (useContext is killer), but I have to wait for official and stable support from mobx-react to migrate.

Well, observer has worked with a HoC-like API for as long as I've been using MobX and it's been fine ever since. I don't see the point of introducing a new API that will only confuse people into which one they need to choose. Especially if it's less capable than the original.

Less capable how? It's a low-level API sure, but it has exactly the same capabilities and it's slightly more verbose and apparent from the code. The HOC tends to be rather hidden.

Well, "less capable" wasn't maybe the right thing to say. I was referring to the gotchas there are when using useObserver instead of observer (you have to declare observable state inside its closure instead of at the root on the component), and to the fact you can only have one by component compared to independent <Observer> blocks (well you can have several but they'd all result in the rerendering of the whole component, so that's not very useful).

I have never written a reaction that has a dependency on a non observable prop, how would it even work?

Consider for example you need a conditional reaction that depends on a prop value. Since you cannot just call the hook conditionally, you have to put that condition inside the reaction. And that means the reaction needs to be disposed and recreated when dependant prop changes. Otherwise the reaction would be always seeing closure value when it was created first time.

Props are observable in class components, so reactions and computed values just worked fine there. This isn't possible with hooks, so I guess you have a point there. Actually this is a major problem since it's a regression in functionality between class and hooks that we can't solve. Specifying props dependencies by hand while observable ones are automatic will be confusing, especially because up until then i didn't have to. I am not sure that we can do anything do solve this though.

@danielkcz
Copy link
Collaborator Author

danielkcz commented Mar 27, 2019

IMO the hooks should have the same API as the original function (observable, autorun, reaction…), and as a hook useObservable should allow for "lazy" initialisation. That's all, and that's what I meant by saying "unopinionated": this is a 1-to-1 translation.

That sounds like your opinion 😆 And I tend to disagree. So you see, there are multiple opinions, thus it's opinionated and cannot exist in its current form 😎

And since hook-based components can be stateful, then we need something that allows us to declare MobX state in that.

I am still not convinced of this. There is close to none performance gain from it compared to React.useState for simple cases. Perhaps I am doing it all wrong, but so far it worked great to have MST in Context for anything "heavier" and for local component state React works just fine and it does not suffer from the need to have observer there. As long as we cannot provide reliable checks if you did not forget to use it, it's a fairly risky API and can cause strange bugs. I got burned by that couple of times already.

I was referring to the gotchas there are when using useObserver instead of observer (you have to declare observable state inside its closure instead of at the root on the component)

This is actually something we are trying to solve in #97 and I think it's getting somewhere. If that works out, it will become much more powerful as you won't ever need to think again about including observer manually somewhere.

Specifying props dependencies by hand while observable ones are automatic will be confusing, especially because up until then i didn't have to. I am not sure that we can do anything do solve this though.

@xaviergonz Was kinda trying to solve this with some useObservableProps hook in the past, but it felt rather bad back then. I am sure we can figure out something eventually. The Macros are a big promise here as it can unburden a from a lot of manual tasks (= human errors).

@JabX
Copy link

JabX commented Mar 27, 2019

And since hook-based components can be stateful, then we need something that allows us to declare MobX state in that.

I am still not convinced of this. There is close to none performance gain from it compared to React.useState for simple cases. Perhaps I am doing it all wrong, but so far it worked great to have MST in Context for anything "heavier" and for local component state React works just fine and it does not suffer from the need to have observer there. As long as we cannot provide reliable checks if you did not forget to use it, it's a fairly risky API and can cause strange bugs. I got burned by that couple of times already.

Computed values are love, life and everything else. Being able to update state without triggering an update when it's not needed can be a huge performance gain, especially when dealing with expensive renders like with lists. Or managing children state in the parent because the parent might need to update it (let's say a list with selectionnable items, where you can toggle each element separately or toggle all of them at once), where you can do all of this without rerendering the list a single time. You can't do this with pure React unless specifying a ton of sCU handlers by hand, that still don't exist for hooks btw.

I don't necessarily want to use external state for this, it can work but it's a bit more ceremony than I'd like (especially if I need context, why would I need context for a simple self contained component that renders a few children?).

Maybe the thing that we (I?) need is a simple hook that allows us to instantiate anything lazily on the first render, and then never touch it again while simply getting back the thing at each render. Something like an immutable ref, without the wrapping object and business with the initialisation. This is what useObservable already does, but there is no reason for it to be limited to an observable object. It could be anything. Such hook shouldn't come from mobx-react since it's way more generic than that though.

Anyway, we are debating over something that is totally trivial when using a class, so maybe that's a sign that hook components aren't really the end of it all... ? (please React team please reconsider bring hooks, and by hooks I mean useEffect and useContext, to classes... that would solve all of our (my?) problems…)

@joenoon
Copy link

joenoon commented Mar 27, 2019

I've been thinking about a version of useObservable that was basically like below, with the same argument signature of useState, but different return signature (only need value):

function useObservable(inValOrFn) {
  return useState(() => {
    const val = typeof inValOrFn === 'function' ? inValOrFn() : inValOrFn;
    return observable(val);
  })[0];
}

I have found uses for wanting lazy observable creation, even if it some cases its just because it feels cleaner knowing I'm not re-running things that will never result in a change.

I like the current useComputed implementation and I haven't seen the downsides that have been mentioned. Maybe this is because its being used outside of useObserver with patterns that make the component and expectations harder to reason about (Observer component, etc).

I'm personally not feeling anything lost with the move to hooks - in fact quite the opposite. I'm hopeful #97 might even make it so much more simple when compared to classes/decorators/hocs/Observer-render-children.

It does seem like with useComputed/useObservable and other mobx-related things we can come up with that at one level they are really small and trivial and people could just implement them in their own codebase. On the other hand if everyone starts having their own slightly varying implementations that has other downsides. It could just be a phase because hooks are still newish. A place (either this repo or a sibling repo) for common hooks where we can fine-tune them would have value to me.

@danielkcz
Copy link
Collaborator Author

danielkcz commented Mar 28, 2019

I wasn't really concerned about performance in any apps. Even without optimizations React is fast enough and we haven't heard from any customer saying "it's slow". That makes me kinda biased in here I suppose.

Maybe the thing that we (I?) need is a simple hook that allows us to instantiate anything lazily on the first render

Yea, what @joenoon said, you already have that lazy init with useState. The exported useObservable might feel way too magical and people don't really know what is it doing. If we could instead tell them to do const [obs] = React.useState(() => mobx.observable()), they would have a complete control and understanding what's happening there. They can even choose to use observable.box or observable.map if they like.

I really want to start making a site dedicated to MobX in React. Explain these recipes instead of giving them some prebaked (and opinionated) solution.

A place (either this repo or a sibling repo) for common hooks where we can fine-tune them would have value to me.

That certainly something I am considering along with that site. Some kind of reference implementation for basic cases and have recipes to let people make their own for advanced stuff.

@mweststrate
Copy link
Member

mweststrate commented Apr 18, 2019

Sorry, found it indeed, wasn't paying attention

  1. This is working as expected, the counter.count is read outside a reactive context, which indeed isn't supposed to react. This and similar cases are extensively described in https://mobx.js.org/best/react
  2. Same
  3. This is basically the implementation of <Observer> :)

To clarify on 1, nobody is tracking the result assignment, since it is not part of any observer component. If in contrast, that would be in-lined in the callback, or done in the callback, or stored as expression (result = () => stuff) that would all address the issue. But in your current setup no recalculation happens if the outer component doesn't render.

This is one of the reasons why "derive, don't cache" is prime in the MobX philosophy. const result is a local cache introducing unnecessary local state. (I get that this is contrived, but when running in weird issues with MobX, it is usually because ppl deviate from the philosophy)

@joenoon
Copy link

joenoon commented Apr 18, 2019

Fair enough, thanks for checking it! I think I'm trying to get at something a bit different that maybe isn't best in this thread. The reason it doesn't work is clear to us in that simple example, but in practice this is the type of thing I know I'll be seeing bug after bug in and having to constantly explain/fix. I think the philosophy of "when in doubt use observer" etc from mobx-react is a great approach - it eliminates a whole category of bugs. So I guess to sum it up, yes this all technically works as designed, but I was attempting (maybe poorly) to get at simplicity... i.e. should those patterns where you really have to know what you're doing be reserved for lower-level areas or do we want to be encouraging all those patterns throughout an entire app.

@danielkcz
Copy link
Collaborator Author

Ok, I think we got too much off the track here. We can generally agree that all three variants of the observer have some use cases and should be kept. It's merely the question of establishing some patterns and recipes which I want to tackle in #115, but could use some help for sure as I am kinda swamped with other stuff.

@mweststrate
Copy link
Member

Fair point, it's all unrelated to the utility hooks :)

@danielkcz
Copy link
Collaborator Author

danielkcz commented Apr 23, 2019

So I just recently discovered an interesting platform for investigating pros & cons. Made one regarding observer pattern, so let's try that :)

https://www.kialo.com/differences-with-mobx-observer-pattern-in-react-28353

danielkcz pushed a commit that referenced this issue Apr 28, 2019
* Wrote docs for the new hooks proposal in #94

* docs: small improvements

* rudimentary, untested, first implementation

* Removed weird exception

* Implemented basic tests

* Add test for effects

* Processed some review comments

* Removed old hooks. Removed / updated tests

* Updated docs

* Fixed stupid linter error.

* Revertd removed hooks, added warnings instead

* One stupid lint rule less

* Reverted old docs

* Added tests counting re-renders

* Apply suggestions from code review

Co-Authored-By: mweststrate <mweststrate@gmail.com>

* Added performance tip

* Yet another nonsensical lint issue.

* Update src/useComputed.ts

Co-Authored-By: mweststrate <mweststrate@gmail.com>

* Update README.md

* Update README.md

* Update README

* Add test for coverage
@danielkcz
Copy link
Collaborator Author

Merged and published #130 in 1.3.0 version

Accepting PR for 2.0 removing these deprecated hooks.

@sheerun
Copy link
Contributor

sheerun commented Apr 29, 2019

I'm looking at implementation of useAsObservableSource and it seems concerning that new observable is created for each instance of stateful component: https://github.com/mobxjs/mobx-react-lite/blob/master/src/useAsObservableSource.ts#L11

It it really necessary? What about using something like autorun instead?

@olee
Copy link
Contributor

olee commented Apr 29, 2019

useState will run this function only once, because it's an initializer!
So there's no issue with that.

@sheerun
Copy link
Contributor

sheerun commented Apr 29, 2019

This is only within one instance of stateful component. I'm talking about multiple instances on page or multiple different components using the same data source

@danielkcz
Copy link
Collaborator Author

This is only within one instance of stateful component. I'm talking about multiple instances on page or multiple different components using the same data source

Well, that's kinda up to you. If you want to optimize that way, simply store that state inside the context and reuse across the components.

@sheerun
Copy link
Contributor

sheerun commented Apr 29, 2019

Then what's the point of useAsObservableSource if it only allows for tracking changes within one instance of stateful component? We already have useState for it (or useLocalStore)

@danielkcz
Copy link
Collaborator Author

It's useful in conjunction with useLocalStore if you have some values derived from a combination of props and observable state. Read the discussion above, there is brainstorming around this, especially #94 (comment)

@JabX
Copy link

JabX commented Apr 29, 2019

Then what's the point of useAsObservableSource if it only allows for tracking changes within one instance of stateful component? We already have useState for it (or useLocalStore)

If you already have observable data (from a prop, from context or even just a plain "global" observable object), you don't need useAsObservableSource and changes will be tracked as they always have by observer/<Observer>/useObserver or any reaction that you may declare in your component.

This hook's goal is to create an stable (= will not change throughout the life of the component) reference to an object that you create from unstable (= will change at each render) references. The main usage here is to wrap props, so that you can use them in useLocalStore and local reactions created with useEffect without having to recreate them at each render to accommodate to prop changes. This is how hooks are expected to work in general, but this behaviour doesn't work well (and flat out doesn't make any sense with observable data), so we had to find a workaround

@sheerun
Copy link
Contributor

sheerun commented Apr 30, 2019

I've slept on it and I think that introducing useAsObservableSource was a mistake if its primary purpose is supporting useLocalStore. A more "react hooks" way would be to pass property dependencies as second argument as so:

    const store = useLocalStore(() => ({
        count: 0,
        get multiplied() {
            return multiplier * this.count
        },
        inc() {
            this.count += 1
        }
    }), [multiplier])

I also believe that react does this convention on purpose because they plan to extend compiler support (just like newest version of svelte does). So while you might consider such "manual" dependency declaration unpleasant, it isn't necessairly forever as mobx-react-lite could introduce in the future babel plugin that auto-injects property dependencies into useLocalStore as well

@mweststrate
Copy link
Member

mweststrate commented Apr 30, 2019 via email

@sheerun
Copy link
Contributor

sheerun commented Apr 30, 2019

It doesn't need to you if implement it this way. This just API change

@sheerun

This comment has been minimized.

@danielkcz
Copy link
Collaborator Author

@sheerun In case you would make it like a dependency, there is no way for you to apply that changed value except throwing away whole observable and creating a new one. We would need to provide a way for you to actually merge with the previous state.

For someone who is starting with MobX you are full of ideas :D

@sheerun
Copy link
Contributor

sheerun commented Apr 30, 2019

I don't know from where the impression that I'm starting with mobx. It's almost 3 years for me..

@urugator
Copy link

urugator commented Apr 30, 2019

@sheerun Also note that:
Mobx is all about automatic subscription management (figuring deps out for you).
Deps array is (optional) optimization tool, not a semantic guarantee - that wouldn't be true in this case as missing deps could cause staleness.
If compilation becomes a necessity, more profound solutions are possible (like svelve, elm, effectfuljs...).

@sheerun
Copy link
Contributor

sheerun commented Apr 30, 2019

#142 Introduces what I'm telling about without "resetting counter" and without "throwing away whole observable and creating a new one"... little faith please

@mweststrate
Copy link
Member

@sheerun apologies, your earlier code snippet suggested / implied a full reset to me without further explanation. The PR is much clearer :).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests