mobxjs / mobx-react-lite

Lightweight React bindings for MobX based on React 16.8 and Hooks
https://mobx.js.org/react-integration.html
MIT License
2.13k stars 90 forks source link

Get rid of (almost) all utility hooks #94

Closed danielkcz closed 5 years ago

danielkcz commented 5 years ago

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: https://github.com/mobxjs/mobx-react-lite/issues/94#issuecomment-482533778


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 commented 5 years ago

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):

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

I have the feeling how I'd actually use mobx inside react with hooks 90% of the time would be something like (for internal state)

const Component = memo(function (props) {
  const mobxProps = useObservableProps(props); // to use props the mobx way

  const [data] = useState(() => observable({
    x: 5,
    // some actions like setX, some computeds // might use mobxProps
  });

  // mobx effects
  useDisposable(() => reaction(() => controller.x, () => { ... }) // might use mobxProps

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

so maybe an actually useful hook would look something like

const Component = memo(function (props) {
  const obsProps = useMobxProps(props);

  const state = useMobxState(() => {
    x: 5
  });

  // obsProps used inside computed inside the class would just work (tm)
  const views = useMobxViews(() => {
      get xPlus1() {}, // computeds
      viewFunc(foobar) // non action functions
  });

  const actions = useMobxActions(() => { // only needed in strict mode really
    // setX
  });

  useMobxEffects(() => [
    // no need to pass props.x to some stupid deps array, no need to re-create the reaction
    reaction(() => state.x === obsProps.x, () => {...});
  ]);

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

or alternatively

class ComponentState {
  // data, views, actions, etc
}

const Component = memo(function (props) {
  const obsProps = useMobxProps(props);

  // obsProps used inside computed inside the class would just work (tm)
  const [state] = useState(() => new ComponentState(obsProps))

  useMobxEffects(() => [
    // no need to pass props.x to some stupid deps array, no need to re-create the reaction
    reaction(() => state.x === obsProps.x, () => {...});
  ]);

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

that's why I thought the proposal I made to make a hook to turn props into an observable was key to break out from react silly state management for good (and actually the result of that is what I'd pass by default to the inner component when using observer, to better mimic what observer does in mobx-react)

danielkcz commented 5 years ago

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 commented 5 years ago

What would you say to at least write a hook to make props an observable ref?

I think it is just this though (at least to imitate what mobx-react does):

function useObservableProps(props) {
  const [obsProps] = useState(() => observable.ref(props));
  if (obsProps.get() !== props) { obsProps.set(props); }
  return obsProps.get();
}
danielkcz commented 5 years ago

Why? :) I had no need for that so far. If you do, then it's perfectly fine to keep it in your codebase.

This issue is about removing stuff, not about adding more of it, keep that in mind ;)

xaviergonz commented 5 years ago

fair enough ;)

JabX commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

This is super interesting thread! Hope to catch up on it later this week. Need to prepare talks and workshops really hard atm 🙈 [image: image.gif] [image: image.gif]

On Tue, Mar 26, 2019 at 10:53 PM Javier Gonzalez notifications@github.com wrote:

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

— 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/94#issuecomment-476867837, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvGhDRtbQ5O6Qp6ipny35HdnucqR23iks5vapblgaJpZM4cELwd .

JabX commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

urugator commented 5 years ago

Observer is more flexible (you can have several independent reactions that don't trigger the main render).

The only way to minimize rendering within bounds of a single component is use of the Observer

Observer has one disadvantage, which curiously enough doesn't seem to be mentioned anywhere. It always re-renders together with parent, because it always receives new children (the closure) and due to the way it's implemented, it unnecessarily runs prop comparison (unless the callback isn't inlined, which is rarely the case).

You can't do this with pure React unless specifying a ton of sCU handlers by hand

There is definitely a lot more ceremony (well arguably, because you don't need a library), but far from specifying a ton of sCU. The state needs to be normalized so that the modification of item doesn't modify the array:

{
  itemList: [0,1,2],
  items: {
    0: { selected: false },
    1: { selected: true },
    2: { selected: false },
  },
}

I share some scepticism about useObserver with @JabX. Not sure whether it should (not) be publicly exposed, but I think the observer should be preferred as there are less opportunities for introducing an error.

The utility hooks are obviously opinionated (proven by the comments and issues). They are also very trivial to implement, mostly a sugar without meaningful logic. I think there isn't enough value in them to be part of the library.

As for the recipes website, I think the main concern (aside of usability) should be the ease of contribution. Probably not suitable, but iodide caught my attention recently.

danielkcz commented 5 years ago

As for the recipes website, I think the main concern (aside of usability) should be the ease of contribution. Probably not suitable, but iodide caught my attention recently.

Thanks, but that doesn't seem suitable indeed. I am currently thinking about GitBook, it seems it got revamped. Not that have been working with previous versions, but it does seem to have some interesting appeal. I am not sure about ease of contribution there though.

I share some scepticism about useObserver with @JabX. Not sure whether it should (not) be publicly exposed, but I think the observer should be preferred as there are less opportunities for introducing an error.

Well, on the contrary, if we succeed with #109, it could be a far better option simply because it will guard you against using observable within a component without having an observer which can be an ugly source of bugs.

danielkcz commented 5 years ago

@urugator

Observer has one disadvantage, which curiously enough doesn't seem to be mentioned anywhere. It always re-renders together with parent, because it always receives new children (the closure) and due to the way it's implemented, it unnecessarily runs prop comparison (unless the callback isn't inlined, which is rarely the case).

Are you sure about that behavior? Can you open a new issue with a relevant example, please? I am not using that component much so I did not notice it really. Ideally, if you have some suggestion on how to fix that, go ahead with PR. Wouldn't be enough to just wrap it into React.memo same as the observer?

JabX commented 5 years ago

Having thought and fiddled a bit about all of this, I'm okay with using useState with observable (like const [obs] = useState(() => observable(…))), because it's not that much more than a specialized hook that will never satisfy everybody, and it can do everything everybody needs from an observable object. But I still think that using state in a function is a hack and that classes still have their uses, but people will still disagree I guess.

Unless we can find a way of building reactions that can react to props, I guess we're fine with useEffect for reactions (even with the empty array), since it's a known pattern that's used for all kinds of side effects. I'd still like to have something more integrated than:

const [disposer] = useState(() => autorun(() => …));
useEffect(() => disposer, []); 

to have autoruns (or reactions with fireImmediately in general) run during first render and not after it, like we do today with @disposeOnUnmount. I think there is maybe something the library can provide to help with that, be it the one utility hook that we have. disposeOnUnmount is a precedent for this, so it wouldn't look out of place.

useComputed is trivial to reimplement (even without hooks), so it can go.

useObserver is still redundant (even <Observer> kinda is, as seen in the above issue), and using macros is definitely not something to encourage IMO.

danielkcz commented 5 years ago

to have autoruns (or reactions with fireImmediately in general) run during first render and not after it,

I am not sure why would you need that. It might even cause weird behavior when you expect the UI to be already rendered, but it isn't. Go ahead if you feel safe with it though :)

useObserver is still redundant (even <Observer> kinda is, as seen in the above issue), and using macros is definitely not something to encourage IMO.

Funny, I find the observer redundant and useObserver to be necessary ;) Well, since you are promoting classes and discouraging macros, I think I got a good picture about your coding habits. No offense :) You are just missing out a good stuff ;)

joenoon commented 5 years ago

I find useObserver to be the critical piece (especially with the macro syntax, but that is icing on the cake) and I think we can do away with <Observer /> and observer in this hooks-specific library, or at least strongly discourage using them. I can't see a reason to use them that can't be done simpler with useObserver. Is there a case?

urugator commented 5 years ago

No offense :) You are just missing out a good stuff ;)

Isn't the excessive amount of excitement a reason for this issue's existence?

Is there a case?

Everything outside of useObserver runs outside of tracking context, it's trivial to introduce staleness. Such bugs are impossible with observer. The worst case scenario is, that you are passing plain values instead of observables, falling back to standard react behavior.

joenoon commented 5 years ago

Everything outside of useObserver runs outside of tracking context, it's trivial to introduce staleness. Such bugs are impossible with observer. The worst case scenario is, that you are passing plain values instead of observables, falling back to standard react behavior.

That's what I mean by is there a case... is there a case for intentionally doing reactive things outside useObserver? I haven't come across one in the apps I'm working on which is why I ask.

Because if there is not, we should just make this all a lot simpler and not have Observer or observer, and show the useObserver pattern (not the one where it is the last return, but the one where it is the only statement).

With the macro in #109 useObserver is basically a 1:1 of what the observer HOC does, just in hook-style: https://github.com/mobxjs/mobx-react-lite/pull/109/files#diff-04c6e90faac2675aa89e2176d2eec7d8R154

class Foo extends React.Component {
  render() {
    return ...;
  }
}
const Bar = observer(Foo)

vs

// regular version
const Foo = () => useObserver(() => {
  return ...;
});

vs


// macro version
const Foo = () => {
  useObserver();
  return ...;
};
danielkcz commented 5 years ago

I believe this is what most people don't realize yet...Instead of thinking about useObserver as a hook that should appear the last in a component it can be actually the very first.

// regular version
const Foo = () => useObserver(() => {
  return ...;
});

There is one slight difference though. The observer wraps the component into a React.memo. useObserver cannot do that. That means the component is re-rendered (and useObserver executed) even if props don't change. Sure, you can wrap the component with React.memo by yourself which is nice that you are not forced to it. But it's definitely less developer friendly in that matter.

const Foo = React.useMemo(() => useObserver(() => {
  return ...;
}));

We could as well expose something like createObserved which would wrap these two tasks together. Suddenly it gives a feeling it's like observer HOC without unnecessary component nesting 😎 But it feels like another utility that can be introduced in userland if someone likes it so let's not get carried away.

const Foo = createObserved(() => {
  return ...;
});

Edit: This also has another advantage. The ESLint won't complain about it because there is no use prefix. With the example above, the linter will complain as you cannot call hooks inside a callback.

Could (or should) macro wrap a component to memo as well?

joenoon commented 5 years ago

Correct me if I'm wrong but React.memo is the function component equivalent of PureComponent.
I haven't used it much but maybe should. But React.useMemo is something entirely different - a generic way to memoize something when using hooks. So would it just be this (which seems pretty good as-is)?

// regular version
const Foo = React.memo(() => useObserver(() => {
  return ...;
}));

// macro version
const Foo = React.memo(() => {
  useObserver();
  return ...;
});
danielkcz commented 5 years ago

@joenoon Everything you've said is correct :) That's the silver lining that you can decide if you want to optimize the component. The observer forces you to do it.

JabX commented 5 years ago

to have autoruns (or reactions with fireImmediately in general) run during first render and not after it,

I am not sure why would you need that. It might even cause weird behavior when you expect the UI to be already rendered, but it isn't. Go ahead if you feel safe with it though :)

It's not a question of feeling safe or whatever, my reaction might need to run during first render because it initializes some state that I need to render the component, that can be updated later on using the same initialization. I am not pulling this example out of my hat or anything, I have working code today that does this and it's more convenient that way, especially since one of the promises of hooks is to reduce the multiplicity of component* hooks in classes.

Well, since you are promoting classes and discouraging macros, I think I got a good picture about your coding habits. No offense :) You are just missing out a good stuff ;)

How am I supposed to react to that? Aren't we literally on the thread discussing of the "good stuff"? If anything, you are missing on the good stuff by refusing to even consider classes to write certain kinds of components or for certain patterns that are definitely easier to write and understand with it. I am not "favoring" classes, I want to use hooks like most people, but there are no shortage of examples over the internet of trivial things to express with classes that are a nightmare to reason with with hooks, especially around state in general.

I just wish the React team weren't blind to this and would decide to go back on a few points (multiple contexts in classes please...)

urugator commented 5 years ago

@joenoon

is there a case for intentionally doing reactive things outside useObserver?

But that's exactly the problem. useObservable allows you to access observables unintentionally outside of reactive context, increasing a surface area for severe bugs for no good reason. Whats more severe problem in a state management library than something becoming stale? Without any clue from where the problem originates. This can't ever happen with observer, because you simply can't access observables outside of reactive context.

a lot simpler and not have Observer or observer

You can also make it simpler in the same way by not exposing useObserver

If you want to do a comparison be fair:

const Foo = observer(() => {
  return ...;
})

// VS

const Foo = () => useObserver(() => {
  return ...;
});
danielkcz commented 5 years ago

@urugator I don't follow what "fairness" are you talking about in that example? It's the same as @joenoon has shown and it's in my examples as well. Or is there really something I am not seeing? Please be more specific.

But that's exactly the problem. useObservable allows you to access observables unintentionally outside of reactive context, increasing a surface area for severe bugs for no good reason.

Sure, it's a problem if you learn wrong patterns. Having useObserver in the middle of the component body can surely introduce bugs. But, if you learn it's actually ok to wrap entire component body into it, there is no problem anymore. Or you can just use macros (once polished) to guard that for you and the problem is no more.

It's definitely an initial mistake coming from this package. We originally thought that useObserver is more like low-level API. Turns out it's much more and it's a valid opponent to observer.

urugator commented 5 years ago

It's the same as @joenoon has shown

No, he's inappropriately comparing class based component to functional component, exaggerating the difference.

it's in my examples as well

Afaik createObserved is exactly how current observer is implemented. There isn't any unnecessary component nesting.

Having useObserver in the middle of the component body can surely introduce bugs.

The hooks were introduced to solve an incomposability of classes. The presented pattern is non-idiomatic as it's not composable (technically is, but practically isn't).

it's a problem if you learn wrong patterns

Or you could try to design the API in way which disallows/discourages wrong patterns.

I simply fail to see why useObservable seems better:

So why should I use it or recommend to anyone? Only because it's hook and hooks are cool? That doesn't make sense...

danielkcz commented 5 years ago

So why should I use it or recommend to anyone? Only because it's hook and hooks are cool? That doesn't make sense...

I am not saying it's a silver bullet. It's a choice.

Personally, I was never a fan of HOC as it's something "outside" the component. It can easily escape your attention. Many times I've either forgot to remove it because it wasn't needed anymore or forgot to add it. For me, the useObserver is just more apparent and feels like a part of the component. Sure, it suffers the same issues, it's just more visible.

it's not shorter to write.

Consider following in TypeScript world

export const MyComponent = observer<IProps & { children: React.ReactNode }>((props) => {
 // this is NOT so extreme case, blame bad typing, but implicit children is not there :(
})
export const MyComponent: React.FC<IProps> = (props) => useObserver(() => {
 // in some cases the useObserver is indeed shorter ;)
 // and it's not so drown in there imo
}

it requires learning some patterns or using macros.

Learning is part of the process, you had to learn rules of hooks as well. They even need a linter to avoid obvious mistakes. It becomes natural after a while.

it doesn't provide any optimizations

Arguably as it's NOT always great to have "implicit" optimization, sometimes it's better to have a power of choice.

it's error prone. it is a hook, but actually doesn't provide any benefit of hooks

We can agree on these I suppose, tradeoffs are everywhere.


There is one thing we have been planning with macros and I still want to pursue that eventually. It would turn useObserver into a powerful weapon. Idea is that you would have to useObserver to be able to access and use hooks that are somehow working with observables. That way you would be completely safeguarded against a misuse. @joenoon Had proof of concept at one moment but then decided to pursue a different direction.

  const { useObservable, observerHooks } = useObserver();
  const foo = useObservable({foo: 1});
  const mainStore = useMainStore(observerHooks);
joenoon commented 5 years ago

. @joenoon Had proof of concept at one moment but then decided to pursue a different direction.

Yes, it's definitely do-able, but I started seeing lots of downsides to that approach:

My current state of thinking and current state of the PR is since people are OK with and understand observer, they should equally be ok with useObserver macro, since its exactly the same scenario - if you don't use it, it won't work... if you do use it, it will work. This also makes typings a breeze which are critical to me.

The macro has two other benefits related to eslint:

Whether or not these are rule problems or real problems or somewhere in between is a different discussion and there are workarounds for useObserver in its current form by using a named function, since we know its called synchronously. But the nice part about the macro is it lets you write normal simple hooks style and keep all the linting intact.

mweststrate commented 5 years ago

Finally found the time to read the above thread and #97. I think at this point I agree with @urugator that useObserver is not the best thing to expose, it has some edge cases and doesn't solve anything <Observer> doesn't.

For state, I noticed that I use the pattern const [obs] = useState(() => observable(…)) a lot. Long story short, this is what I would propose at this moment. Shoot!

Creating observables

const state = useObservable(() => someObject, decorators? | deps?, deps?)

Creating side effects

Creating observer components

I think using observer or Observer are pretty great in their own way, and useObserver is not that great from composition perspective (as-last, and hooks-nesting or both not optimal as discussed in #97), and wrapping on the outside doesn't yield real benefits over observer? (unless I missed something in the above thread). Auto applying memo hasn't caused any bug report as far as I can remember in the last 3 years, there seems to be no real need to bail out of that. In immutable architectures the optimization might cost more than it saves (if the props always change, which is why it isn't the default), but in MobX this is much less so, since properties typically don't change that much as object references are stable, meaning that memo hits cache in the far majority of the cases, as soon as anything observable is passed in.

Note that, if return <Observer>{() => rendering}</Observer> is considered to be to much typing, we could also make the api more convenient like return observed(() => rendering). (or: track / tracked / observing) That is almost the same as useObserver. Except, it composes a bit more cleanly inside a rendering tree, as the following would be valid without triggering lint rules. Example component


const PageRenderer = ({ page, maxTitleLength }) => {
  const pageView = useObservable(() => ({
      selected: false,
      get displayTitle() {
        return page.title.substr(0, maxTitleLength)
      },
      toUpperCase() {
        page.title = page.title.toUpperCase
      }
    }),
    [ page, maxTitleLength ]
  )

  useEffect(() => reaction(
      () => page.toJSON(), 
      data => saveToServer(data)
    ),
    [ page ]
  )

  return (
    <Section>
      <h1>{observed(() => pageView.displayTitle)}</h1>
      <h2>{observed(() => page.subTitle)}</h2>
      <button onClick={pageView.toUpperCase}>Uppercase</button>
    </Section>
  )
}

(N.B. Obviously one observed could have been used here)

danielkcz commented 5 years ago

I think at this point I agree with @urugator that useObserver is not the best thing to expose, it has some edge cases and doesn't solve anything <Observer> doesn't.

It would feel extra weird wrapping a whole component code to the <Observer>. It's a very unusual pattern. The hook is slightly better on that (especially with macro) imo. Personally, I hate to see any observer mentioned in react devtools. Call it OCD if you like :)

  • Because props are not observable, one might need to specify deps to make sure a fresh observable object is created if certain props change

And I think this is more than wrong. It means it would reset whole state when a single value change and possibly losing some other state. It's more close to the useMemo behavior than a useState and without any control when an update happens (no access to the previous state).

and useObserver is not that great from composition perspective

On the contrary, only the useObserver can be part of the custom hook and make it observable on its own without relying on the component being observable. How is that for a composition perspective? :)

but in MobX this is much less so, since properties typically don't change that much as object references are stable, meaning that memo hits cache in the far majority of the cases, as soon as anything observable is passed in.

Not sure how is that related. If you have MobX stores in the Context (or even with useObservable), there can still be a bunch of props that change (especially scalar ones). Not that it's bad to have memo there, but it's not such a win all the time. With useObserver I can actually decide when I want to optimize.

as the following would be valid without triggering lint rules

Mentioning that, the lint rules goes totally blind (for a whole component) if you use observer HOC. Haven't really investigated if it's an issue on their side or something in here, but it's a fact. The useObserver does not suffer from that and with a named callback, it works just fine.

danielkcz commented 5 years ago

Btw, as much as observed looks nice, it's just another sugar for the same thing. If we would be keeping <Observer>, it's too much API choices already. Besides, it does not solve the issue when you have observable within a function body. With GraphQL it's very common to have this.

export const LogoutWidget = () => useObserver(() => {
  const { auth } = useRoot() // MST
  const { data, loading } = useQLogoutWidget(
    { id: auth.userId }, // this is observable
    { skip: !auth.isAuthenticated }, // and this
  )

  let displayName
  if (loading) {
    displayName = i18n.t`Not logged in`
  } else {
    displayName = `${data.user.firstName} ${data.user.lastName}` // this too
  }

  return (
    <Link to="/logout">
      <div>{displayName}</div>
    </Link>
  )
})
urugator commented 5 years ago

const state = useObservable(() => someObject, decorators? | deps?, deps?)

If we would want to match react's hooks behavior, user would always have to provide at least empty deps array: const state = useObservable(() => {a:'x'}, []) Perhaps it's a bit too overloaded with functionality.

urugator commented 5 years ago

On the contrary, only the useObserver can be part of the custom hook and make it observable on its own without relying on the component being observable. How is that for a composition perspective? :)

Do you have an example? useObserver returns an element, therefore a custom hook using useObserver would also have to (?) return an element, which makes it sort of a "component" with a lifecycle bound to consumer. Eg:

function App() {
  // Note the hook limitations - no conditions/loops etc - are they suitable for something like this?
  const header = useHeader(headerProps);  
  const body = useBody(bodyProps);
  const footer = useFooter(footerProps);

  return el(React.Fragment, null, 
    header,
    body,
    footer,
  )
}
danielkcz commented 5 years ago

useObserver returns an element

Are you sure? It returns any output from a callback you pass in there (and wraps it into a Reaction). It doesn't have to be an element.

https://github.com/mobxjs/mobx-react-lite/blob/5c07ae05282088b553205d3754a784ed1523c8bb/src/useObserver.ts#L55

I don't have any viable example right now, but I know it works.

urugator commented 5 years ago

Obviously you can return whatever, but it seems to be designed for rendering. I am just asking how it can be composed in practical way.

JabX commented 5 years ago

The deps array argument for useObservable would be a good reason to have a dedicated hook for creating observable objects (as opposed to using useState), but I share both @FredyC and @urugator's opinions about it, being that resetting the entire state at each dep change might seem extreme, and that it doesn't share the same behaviour than other deps array arguments.

I still think that there's a great use case for that in computed properties (that might/will be declared inside useObservable), so maybe the deps array could only be used to reset those properties (if that's even possible or make sense)? Maybe the real hook we need in the end is useComputed and not useObservable?

The more I think about it, the more I think useEffect isn't the right way to attach reactions to components, because:

There might be ways to circuvent these problems in userland, but I think that providing the solutions in the library is a good way to help people avoid shooting themselves in the foot.

mweststrate commented 5 years ago

It would feel extra weird wrapping a whole component code to the <Observer>

It's a small downside, but I think it is one of the least concerns personally. Actually, it might also have benefits, as we can soon leverage the React devtools to show the dependency tree of an observer component

It means it would reset whole state when a single value change and possibly losing some other state.

You are right, it's weird. Crunched on it a couple of days, and this is stupid. Better proposal below :)

Mentioning that, the lint rules goes totally blind (for a whole component) if you use observer HOC

I hope that is a temporarily bug? And I think it could be prevented if needed by lifting observer to the export statement (if applicable), which also fixes the display name. Anyway, just parking the observer / Observer / useObserver for a little bit now, I did crunch about the useObservable a bit, and I think the essence of the problem is that unlike in mobx-react@5, props and state are not observable, meaning that computed functions and other things don't act properly. But I think I've found a solution, so please shoot!

Basically, useObservable should take the following form:

useObservable(props, props => observableObject)

This fixes a few problem:

  1. The hook can create and reuse an internal observable object, storing the props for the entire lifecycle of the component. When the hook is called again, that existing observable object is updated form the provided props.
  2. This means that the second closure, which initializes the observalbe object, is allways called with the same (observable) props object, and safely use it in computes. For all clarity, this function is called only once! but the param it receives, is continously being updated as new props arrive (the first arg)
  3. This also introduces a nice place to create autoruns and alike, without needing further hooks!
  4. No need to thinks about deps so it is much more mobx-y.

A quick exampe (from my previous comment):

const PageRenderer = ({ page, maxTitleLength }) => {
  const pageView = useObservable({ page, maxTitleLength} /* or just props */,  ({ page, maxTitleLength }) => {
    const pageView = {
       selected: false,
       get displayTitle() {
          return page.title.substr(0, maxTitleLength)
        },
        toUpperCase() {
          page.title = page.title.toUpperCase
        }
      }

    // start some effects as well if desired
    const disposer = reaction(
      () => page.toJSON(), 
      data => saveToServer(data)
    )

    return [pageView, disposer] // just returning pageView would be ok as well, and any function returned would be assumed to be a disposer
  }) /* no deps! */

  return useObserver(() =>
    <Section>
      <h1>{pageView.displayTitle}</h1>
      <h2>{page.subTitle}</h2>
      <button onClick={pageView.toUpperCase}>Uppercase</button>
    </Section>
  ))
}

In principle it is even possible to combine React state and props into this object, if you also want to react to React state, like:

function WeirdComponent(props: { multiplier: number }) {
  const [count, setCount] = useState(0)

  const counterDoubler = useObservable({...props, count}, ({ count, multiplier }) => ({
    get doubleCount() {
        return count /*state*/ * multiplier /*prop*/
    }
  }))

  useObserver(() => <div>{counterDoubler.doubleCount} </div>)
}

To simplify things the following overloads should also be possible

useObservable<P, T>(props: P, (observableProps: P) => [T, ...IDisposer[]] | T): T
useObservable<T>(() => T): T
useObservable<T>(T): T // convert plain object into an observable

Implementation wise, it could be roughly something like:

function useObservable(props, fn) {
    const [observableProps] = useState(() => observable(props, { deep: false }))
    const [baseResult] = useState(() => fn(observableProps))
        Object.assign(observableProps, props) // update observable with latest props    

    useEffect(() => () => {
        if (Array.isArray(baseResult))
            baseResult.slice(1).forEach(disposer => disposer())
    }, [])

    return Array.isArray(baseResult) ? baseResult[0] : baseResult
}

Shoot!

urugator commented 5 years ago

is allways called with the same (observable) props object, and safely use it in computes.

I don't follow, computed/reaction in your example don't depend on some observable props object - the props are destructured in args... If you update this observable props object it won't notify these computeds/reactions, does it?
Btw reactions created in useState callback are also a subject of that custom GC (actually not sure if GC workaround is applicable here, since these reactions can do whatever and therefore should be disposed immediately).

mweststrate commented 5 years ago

If you update this observable props object it won't notify these computeds/reactions, does it?

Your correct, got carried away in enthousiasm. But without destructuring it does work as expected, demo: https://codesandbox.io/s/4lv0z7j159

reactions created in useState callback are also a subject of that custom GC

Correct again :) We could either hook into that GC mechanism, or instead create thunks that create the actual reactions in a useEffect, like:

useObservable(props, props => {
   const createSaveEffect = () => reaction(
      () => page.toJSON(), 
      data => saveToServer(data)
    )
    return [pageView, createSaveEffect]
}