-
|
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 read this before when this issue was "current" but I realize now that I disagree with a lot of things that are said here:
That's not what it does though ? In any case, at no point You can use several
Huh, it's the other way around, conditions and loops cannot contain hooks, because all hooks must always be declared and in the same order. Hooks can do what ever they want with the callback you give them though. I agree that calling hooks inside Me and my coworkers have written hundreds of components with |
Beta Was this translation helpful? Give feedback.
-
Yes, you can avoid function cmp({ x }) {
return useMemo(() => {
return <span>{x}</span>
}, [x])
} It's appealing because the rendering can depend on different subset of props than effects and also it won't re-render on context change, but in the end it didn't turn out to be the most practical to me.
Yes, I wrote that wrong, but it's meant the same way. The original claim was that |
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.
-
|
I've moved this issue to mobx repo (#2530) and converted to discussion)
Frankly, inference and generic arguments in TypeScript doesn't work that well overall. But I believe this is rarely used and you can always do
And using Check my #2566 (comment), especially the second part. I think that's the path we can go, but someone must be willing to contribute that. |
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.
-
|
I agree, I have some deal of guilt on that as I made those docs. But nobody ever attempted to correct me, so that's that. The hooks were new and shiny so it felt right to use those everywhere. Just a hype. It was hard to see all downsides at that point. |
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.
-
|
|
Beta Was this translation helpful? Give feedback.
-
|
you're right. We are thinking again to see if it is ok with everyone to use the hoc observer explicitly rather than hidding stuff in hooks |
Beta Was this translation helpful? Give feedback.
-
|
@JSteunou One thing that might not be obvious at a first glance. If you have some logic you want to do with data and it's something shareable across the systems, then you can extract that logic into a function (not necessarily a hook) and just pass it the values from the store. The function transformColors(currentColor: string, isNightModeActive: boolean) {
// some more logic before returning colors
}
const Comp = observer(() => {
const store = useAppStore()
const colors = transformColors(currentColor, store.settings.isNightModeActive)
})You can also pass the whole |
Beta Was this translation helpful? Give feedback.
-
|
yep, was thinking about this, but the all point was to hide complexity in some cases. But I guess hiding the usage of |
Beta Was this translation helpful? Give feedback.
-
|
Yea totally. Besides, there is a micro-optimization aspect. If you use multiple hooks with |
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.
-
|
I think the problem is much more a conceptual problem rather than a technical problem which causes selection as something reusable to be a conceptual misfit. Take the following, possible simplest hooks: const useTodo = observerHook(id => {
const store = useContext(TodoStoreContext)
return store.todos.get(id)
})
const ShowTodo = ({ id }) => {
const todo = useTodo(id)
return <h1>{todo.title}</h1> // won't react
})The above example break; the const useTodo = observerHook(id => {
const store = useContext(TodoStoreContext)
return toJS(store.todos.get(id)) // will react to absolutely everything that happens to the todo
})So any utility hook can't generically solve what for specific call sites should be tracked. That is because it goes exactly against the design of MobX of precisely tracking what is read. Now off course, designing a specific hook could work that gives exactly what the rendering would need, like: const useTodoTitle = observerHook(id => {
const store = useContext(TodoStoreContext)
return store.todos.get(id).title
})But this has its own problems.
On the other hand, it is trivial to create a generic, reusable and non-mobx aware hook and call it from any observer component: // reusable, efficient, and exact reading patterns don't need to be known upfront
const useTodo = id => {
const store = useContext(TodoStoreContext)
return store.todos.get(id)
}
const ShowTodo = observer(function ShowTodo({ id }) {
const todo = useTodo(id)
return <h1>{todo.title}</h1>
}))Now this approach has none of the downsides described above; it doesn't over subscribe, it doesn't undersubscribe, it doesn't need mitigation rules like the ones described above, and, if you are worried about forgetting observer you can enable the very same flag that to detect untracked reads that you would for the hooks case as well. The conceptually alternative approach that could be taken is to introduce a hook that would proxy observables again, and track the reads in the context of a specific component. For example valtio takes this approach: https://github.com/pmndrs/valtio. But then you get a different tradeoffs: 1. refs to the object aren't stable anymore like in mobx, as a proxy-of-an-object !== object. and 2. what is the effect of passing a proxied object to a child? I guess (didn't verify) that will cause the entire tree that owns the proxy to invalidate, loosing a lot of the MobX efficiency. But anyway that is a nice example of different design tradeoffs being taken, that might be visually be looking more like the API you are looking for. But within the MobX context I don't see how a purely hook based selection API doesn't introduce more problems than it actually solves. |
Beta Was this translation helpful? Give feedback.
-
|
I understand and agree what you say. However I am concerned only about the scenario depicted by @DavidDuwaer - integrating mobx based app/lib/module, into non-mobx react app/lib/module. // This is exposed to consumers as part of our API
function provideTodos(Cmp) {
return observer(props => {
const store = useContext(MobxStoreContext)
return <Cmp {...props, todos: toJS(store.todos) } />
})
} |
Beta Was this translation helpful? Give feedback.
-
That looks absolutely correct and expected to me, the consumer of that hook would never expect the property access to be tracked here, either because they don't know anything about MobX, or because they actually know MobX and thus know that you need
I still think that it's okay that this is possible. 95% of people wouldn't even know it even exists because it would be hidden away in the docs, and 95% of the 5% that know about it would never bother with that because the path of least resistance for dev is to use So, why bother with features that would maybe be of use to 0.25% of users? Well, I'm not arguing that we should support that, I'm just trying to find more examples of stuff that could only be done if the I can understand the maintenance burden of having 3 separate way to achieve the same thing, but here they're just all the same. |
Beta Was this translation helpful? Give feedback.
-
"...to me" is the important part here :) That is awesome, but not everyones understanding MobX as thorough nor do most people read a lot of docs, and our issue tracker and support burden has significantly reduced after removing useObserver from the docs and API. What is more, if |
Beta Was this translation helpful? Give feedback.
-
|
... but did you try already running your own version? This snippet should work fine, is 4 lines to maintain and feel free to call it |
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.
-
|
See comments above, a utility hook like this would to that: And that selector could be made reusable again: |
Beta Was this translation helpful? Give feedback.
-
|
Ah of course. It looks so simple when you know how. |
Beta Was this translation helpful? Give feedback.
-
|
(I just realised that this has been talked about in an earlier sub thread but I had missed it as it was behind a View more link) |
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…