[Discuss] useObserver, utility hooks

[joenoon]

I'm doing some experimentation/refactoring in light of #38 and #94, and just in general catching up. I have a few places where useComputed works quite well. What I'm starting to think is the pattern of useObserver as the last return is actually the root of a few problems.

For one, it means you can never do an early return (since a hook is the last return), which is a huge annoyance to me.

Second, using the useObserver hook as the last return creates a split in the component of unobserved/observed behavior, which I think is just adding complexity.

I've started going back to wrapping components with observer directly and liking how it feels so far. Under the hood it's still using useObserver, but it's around the entire component.

Happy to be completely wrong on this or have my mind changed, but is it possible useObserver is really something that should be more of an internal implementation detail and not exposed to the user?

[danielkcz]

I've been struggling with this for some time as well. But I've also realized it's simply possible to do the following.

const DataFetchingComponent = ({ obs }) => {
  return useObserver(() => {
    const { data, loading } = useQuery({ variables: { value: obs.input })
    return <UiWithDate data={data} />
  })
}

It might feel strange at first, but it doesn't break any rules of hooks in the end. The useObserver does not need to be the last, it's just it needs to wrap around anything you need to observe. It's fairly similar to using observer HOC, but this way your tree stays clean.

[joenoon]

I was doing that in a few places as well for the same reasons but I stopped. Are you sure it doesn't break the rules of hooks?

When I add 'react-hooks/rules-of-hooks': 'error' to my eslint rules I get "React Hook "useObservable" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function."

I turned the rule off for a while just to use the wrap-everything useObserver style, but now I'm trying to turn it back on. If it's valid, maybe this was a eslint rule bug at some point and I just need to update my libs. I will give that a shot.

Update:
I just updated react* eslint* libs are still see this error

[danielkcz]

Well, I think it's a false positive from that rule considering it's actually working in runtime :) Sadly, there is probably no clean way for it to recognize that case automatically. Even though it's a callback, it will be synchronously executed always. I suppose that rule would need to support a configuration for hook names that would be excluded from that rule.

[joenoon]

So a little more digging and it seems:

using observer - the eslint hook rules silently don't check these... which makes it tricky because I didn't know until I intentionally broke some rules.

using useObserver - two problems: the eslint hook rules don't like these at all (callback error), and even if they did (by hacking the rules source) they won't run the hook rules on the inner function, similar to what happens for observer

I'll keep looking for more ways. Ideally the hook rules would be enforced and not silently ignored or loudly complaining.

Update:

It looks like this works:

export const MyComponent: React.FunctionComponent<Props> = props =>
  useObserver(function useHook() {
    return <div>MyComponent</div>;
  });

by naming the inner function like a hook, eslint picks it up. useHook is not an import, its just a random name for the function, it could be useAnything (in case someone stumbled upon this can't figure out where useHook came from). Not a fan of the prettier formatting on this pattern, but thats another story :)

[danielkcz]

I think you should probably discuss this over at react repo. Apparently, some tweaks to heuristics of that rule will be required. We cannot really change this in here :)

[joenoon]

I think it's a bit of both. Maybe the rule could be made to mark a hook that takes a synchronous function as something that should be treated as a hook function itself. Over this weekend I've settled on a new pattern I'd like to share that I think simplifies the usage and reduces gotchas when using mobx-react-lite:

https://gist.github.com/joenoon/abafacb23329e535a459ac037f27a05e

This enforces a pattern around using useObservable and useComputed which I think are entirely useful hooks when used in the expected way (relevant to #94). But they really must be used within useObserver to avoid edge cases, otherwise they increase developer complexity and mental overhead - you would really have to understand how someone developed a component and reasoned about what/when things should be observed without enforcing this pattern. useObservable and useComputed exported from this gist enforce a first argument that must be passed, almost like a context (although it's just a very simply dummy object). This ensures they can only be called from within a useObserver hook. Also notably there is no export of observer or Observer which I haven't found a case for. Components can always be split down to smaller and smaller parts with what React already provides if only very specific parts need observability - these smaller/specific components can then use this useObserver pattern.

I've refactored my app to use this pattern and am happy with the simplified experience and reduced edge cases so far, but also eager to hear if anyone sees downsides to this approach.

[danielkcz]

Hm, I don't really understand this part...

   useObserver(function useHook(isObserved) {
     const someObservable = useObservable(isObserved, {foo: 10});
   }

Since you already know it is wrapped in the useObserver, why do you feel the need to pass the check in the useObservable? It's fine if you like it, but it's too verbose a clutters the code in my opinion.

What if useObserver would be actually returning those other hooks bound with that check internally?

   const { useObservable } = useObserver(function useHook() {
     const someObservable = useObservable({foo: 10});
   }
   // or 
   useObserver(function useHook({ useObservable }) {
     const someObservable = useObservable({foo: 10});
   }

Although it's almost too much of use word on a single line. Also, I was thinking we could fool the ESLint to avoid the need for a named callback. It is slightly strange and of the books, but seems like a reasonable tradeoff.

   return usingObserver(({ useObservable ) => {
     const someObservable = useObservable({foo: 10});
   }

Looks like some interesting and tidy pattern is emerging here :)

[joenoon]

To the first part, I don't know it's already wrapped with the current useObservable, so mine enforces it.

To the second part, this is much cleaner, nice! So useObserver in this case would be the only hook export, and everything would come off of that with destructuring! I like where your last update is going!

[danielkcz]

Yea, I think I am loving it myself. Not sure yet it should be part of the lib though.

Perhaps the core functionality useObserver & observer can become an integral part of mobx-react V6, after all, to act as a base. And we could rename this package to contain utilities only and perhaps even introduce this pattern there for advanced users.

cc @mweststrate

[joenoon]

FYI I've updated my gist above with this and refactored what I'm working on to this pattern to get a feel for it. I love the simplicity - less imports (only one needed), enforced behavior! Its about a 5-liner if you don't count my comments/example in the gist!

I could not get around the hook rules though with your idea, so I left the named function in until a better way is discovered.

[danielkcz]

I see. I suppose that linter has a problem with useObservable (and others) within a callback function, but when it sees that callback is also named with use prefix, it can safely assume it's synchronous and let it through. It would be probably necessary to do a following, but that's too tricky because you can then accidentally introduce early return and break rules of hooks.

   return usingObserver(({ createObservable, createComputed ) => {
     const someObservable = createObservable({foo: 10});
     if (earlyReturning) {
       return null
     }
     // possible fail here...
     const someComputed = createComputed(...)
   }

I still think that you should try to report this to react repo and perhaps there is some way they would improve this false positive.

[joenoon]

I'm just not convinced its a false positive yet. The current style is really useObserver(anonymousHookFn). Hook functions have names and rules. Anyone using useObserver right now without naming the function useX is unknowingly bypassing the rule checks, so they think they are doing everything right. If they add an early return in front of a hook they will see no error.

What would be really interesting is if we could get the useObserver hook more in line with other hooks. Something like this would be nice, but would require figuring out how to wrap the rendering differently and not sure its possible:

const {useObservable, useComputed} = useObserver();

[danielkcz]

I'm just not convinced its a false positive yet.

It is false positive because linter is failing while runtime code is working correctly. That's kinda rule of thumb there :) On the contrary, just the fact the linter gets satisfied when you have named use function as callback talks about weird heuristics because rules don't mention such escape hatch.

One way could be developing a macro (for babel-plugin-macros). Whenever you would useObserver (anywhere in the function body), it would wrap whole thing it a runtime variant of that hook.

import { useObserver } from 'mobx-react/macro'
const ObservedComponent = () => {
  const [state, setState] = React.useState()
  useObserver() // imported macro
  if (loading) {
    return "Loading"
  }
  return <Content />
}

It would get transformed roughly into this...

import { useObserver } from 'mobx-react' // notice no macro
const ObservedComponent = () => {
  return useObserver(function useObserverRender() {
    const [state, setState] = React.useState()
    if (loading) {
      return "Loading"
    }
    return <Content />
  }
}

I have no experience writing macros or fiddling with AST generally, so it's just theoretical, but would be nice and lazy alternative :)