-
Hey all. Congratulations on the recent releases! I've been happily chugging along with mobx for a while now, and after a relatively pleasant migration to mobx@6 I was a bit disappointed to see that While I recognize the potential for abuse and user-error, I think The benefits of
const useRootStore = (render) => {
return useObserver(() => {
const store = useContext(MyStoreContext)
return render(store)
})
}
As far as Compromise?If the goal is to encourage people to exclusively use it for rendering, and not data selection, perhaps consider naming it In conclusion, I readily admit that there's a lot about mobx that I'm not an expert on. Nonetheless, |
Beta Was this translation helpful? Give feedback.
Replies: 14 comments 29 replies
-
I don't think any new arguments are brought to the table here. That it looks nicer is a pretty poor argument imho to introduce the need to all consumers to understand the difference. Beyond that your above example reeks like an anti-pattern, hooks should be called unconditionally, which is not guaranteed by your nesting without knowing the semantics of useObserver. I can actually imagine the standard React lint rules will fire on your example, although I didn't try that. I think for me your request can be kind of paraphrased as: "A kitchen knife can be used to chop strawberries. But can we use swords as well? Yes it is much more dangerous, but then again, it looks super cool!" (I don't mean that condescending, but witty / light hearted ;-)).
That would be my dream. In practice people barely read docs, and they often file issues before reading docs in depth. This has thought mew over the years to be really wary to put too powerful tools in hands of package consumers, because it usually fires back in the form of a high maintenance burden. Trust me, I'd wish "use this at your own risk" warnings would actually work 😅 FWIW if you insist on having the utility, I wouldn't roll your own, but rather apply patch-package to force an export, so that your logic doesn't get out of date with the original implementation :) (or simply wrap Observer, that will probably work in most cases, but possibly not in all) |
Beta Was this translation helpful? Give feedback.
-
Thanks for the quick reply! I'll admit I was hoping for a different response, but I hope you will at least leave the issue open for another few days to see if anybody else agrees (or disagrees :D).
I considered omitting the "looks nicer" parts of the argument, but included them for completeness. I regret that now, and so I'll reiterate that I believe the primary benefit of
This caught me off guard, since I don't follow how the example is an anti-pattern. If
No offense taken. However, what I meant to get across is that mobx itself offers (in my opinion) a fair number of more "sword-like" public API's that trust that the user knows what he/she's doing. I meant to draw a parallel here.
Totally understand. I happen to be a person who does read through all of the docs, but I can see how frustrating this can be as a maintainer, and I don't mean to imply that you should be forced to support problematic features. I was hoping to point out that there's a grey area here, where for the library to be practical to "power" users there has to be some element of trust.
Thanks, I'll consider that :). And, as always, I truly appreciate the work you and others have done on mobx. |
Beta Was this translation helpful? Give feedback.
-
I was probably the biggest "defender" of
It's actually the number one rule: Call Hooks from React function components.. In your case, you are calling hook from another hook and that's against that rule. Let's put aside it will work simply because it's synchronous flow.
React devtools have a filtering capability for a while, they even allow hiding all HOC. So that's an argument of the past ;) However, I am thinking we could actually keep it exported (without documenting it), and additionally, when used for the first time, it would throw a hard error unless you call something like If you are ok with something like that and you are able to work on a PR, I would give that a gree light. Otherwise, I don't think anyone else would be willing to work on that :) |
Beta Was this translation helpful? Give feedback.
-
Why? Calling a hook from another hook is absolutely correct code.
|
Beta Was this translation helpful? Give feedback.
-
I think @FredyC is referring to https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level:
The hook call here is not a top level statement, but yes it does technically work correct, but only if the assumption is made that useObserver calls it fn always, immediately, and unconditionally. That is the case currently, but things like suspense, debounced rendering, microtasking, worker rendering etc would all kill that assumption. We don't guarantee anywhere that tracking in useObserver happens immediately. So it is not unlikely that in a patch or minor version this changes because we apply some optimization; thereby breaking this kind of utilities. In contrast, the following achieves exactly the same as far as I can see, has none of the downsides, and is imho actually clearer to read:
Obviously lifting the store could also have been done in your original example, and it could also not be done in my solution above, but I think the |
Beta Was this translation helpful? Give feedback.
-
The composability of function cmp() {
// a hook
const render = useObserver();
// not a hook
return render(() => {
})
} Which is less attractive and raises other questions/gotchas/issues. Another problem with this "composability" is that you loose the benefits of mobx's fine grained subscriptions - we cannot invalidate/rerender only single But really the biggest problem is how easy it is for a user to introduce an error, without any hints for solving it. With
I think you can only speak about tree cluttering if nodes in the tree are non-visual and provides just logic/utility. |
Beta Was this translation helpful? Give feedback.
-
Fair points made by all. I appreciate all of the feedback and rest my case for now. I still have some disagreements about the whole "it violates rules of hooks" arguments; however, given the other reasonable points made by everyone above I don't intend to die on this hill. With that, I'll close this for now. If anybody else feels more strongly than me please feel free to bump the issue. Just for the sake of discussion and context, I did want to reply to the following:
I stared at this for a long time and still couldn't quite come to grips with it. I'm intimately familiar with the rules of hooks: React needs the same hooks to be called in the same order each render (i.e. they cannot be conditional/dynamic). Beyond that, your component is free to be as dynamic as it pleases. So, to me it followed that a render function that follows the rules of hooks should be able to be wrapped in another hook. I still don't see any issue with any of that. What I did realize, though, is perhaps what @mweststrate alluded to earlier, and it has nothing to do with the dynamicism of a render function (because it must follow the rules of hooks regardless). Because
I... somehow never knew this, and now feel like a dummy. Thank you, @FredyC. |
Beta Was this translation helpful? Give feedback.
-
My reasoning goes like this: I am only questioning the composability, considering the purpose of the hook and restrictions of hooks. |
Beta Was this translation helpful? Give feedback.
-
I know this issue has been closed for 2 weeks now, but I'd like to add a few more things to the discussion, as I am very much in favor of keeping My main issue with encouraging the use of a My other issue with the Using I agree that it's not the easiest way to get going and that it can get confusing for someone who didn't read the docs. I also agree that its nature as a hook is weird. But at the very least its a "low level" utility that's an export away from being available (I know it's still available behind a deprecation message, but that's bound to change), so it doesn't cost much to keep it. It can be hidden behind a big-ass warning like |
Beta Was this translation helpful? Give feedback.
-
Yes it's annoying, but you have to deal with this constantly even without
The question is to which extend
Possibly they could be redesigned somehow, but in their current shape they should have been deprecated/removed long time ago (imo). |
Beta Was this translation helpful? Give feedback.
-
^ why use memo hoc + useObserver when you can just use observer hoc for the same result. I think the first docs on useObserver caused some confusion. |
Beta Was this translation helpful? Give feedback.
-
Hi, our team is starting to migrate to MobX 6 and was surprise to see the deprecation notice on How would you replace it when used like this? export const useColors = (currentColor: string) => {
const store = useAppStore()
const isNightModeActive = useObserver(() => store.settings.isNightModeActive)
// ... some more logic before returning colors
....
} This one is very simple, but we have other cases with more logics, that we think do not have a place inside store computeds, because only related to local UI / component logic, and result as simple primitive. We could have used Our need is basically a useState of primitive with a reaction pattern. We though Can it be brought back to life just for this case but maybe with another name for it? we thought about something like export function useReaction<T>(fn: () => T, deps: any[]) {
const [state, setState] = useState(fn)
useEffect(() => reaction(fn, val => setState(val), {fireImmediately: true}), deps)
return state
} |
Beta Was this translation helpful? Give feedback.
-
@JSteunou I have been using So, is there any correct way in which we can achieve from a React component something like
without dragging my whole component into it? The thing is, a hook like the above can be used in other hooks, and those hooks can be used in other hooks. Some people have disputed this in this thread, and I'm surprised no-one has yet pointed out that React itself is telling people to do so. The problem other posters mentioned of upwardly communicating the 'hooks contract' of not calling them conditionally is solved with the 'use...' naming convention, and other than that they are plain functions. Composing hooks is a major use case in React: it's the new way to organise your logic. But if mobx relies on me decorating/HoC'ing any component I have used some mobx in (even in some deep subroutine), then that actually breaks the portability of my hooks: I have to tell every colleague to wrap the component in the |
Beta Was this translation helpful? Give feedback.
-
Hi, I just wanted to seek clarification on this issue around a specific use case. Answers here are talking about things like conditional hooks and mutable data. I have a simpler problem: I'd like to expose some read-only properties to other consumers. Previously I have done this by writing custom hooks using |
Beta Was this translation helpful? Give feedback.
Fair points made by all. I appreciate all of the feedback and rest my case for now.
I still have some disagreements about the whole "it violates rules of hooks" arguments; however, given the other reasonable points made by everyone above I don't intend to die on this hill. With that, I'll close this for now. If anybody else feels more strongly than me please feel free to bump the issue.
Just for the sake of discussion and context, I did want to reply to the following:
I stared at this for a long time and still couldn't quite come to grips with it. I'm intimately familiar with the r…