LeetCode-OpenSource / rxjs-hooks

React hooks for RxJS
2.18k stars 83 forks source link

Request: pass in observable directly instead of factory #8

Open cgross opened 5 years ago

cgross commented 5 years ago

Great library. I would be great if I could just pass an observable into the hook directly:

useObservable(myObservable);

instead of

useObservable(() => myObservable);

Brooooooklyn commented 5 years ago

Good suggestion. I wrote the first version of rxjs-hooks in useObservable(myObservable) interface, but then I felt very wired when I was using it.

In a react component, if I want declare a Observable and using it:

function App () {
  const counter$ = new BehaviorSubject(0)
  const value = useObservable(counter$)
  return <div>{counter}</div>
}

the counter$ will redecalre every rerender happened. So, I'm not sure what I should do if the counter$ changed, and I'm not sure what behavior should be if the counter$ changed, should I switchMap or exhaustMap? I don't know.

You can pass the counter$ as props to using it, but it's still a problem what useObservable should do if counter$ changed, and it will be not so easy to use if pass an Observable as props.

But if useObservable receive a callback, I can document it Just run once in componentDidMount, there won't be any misunderstand and ambiguity.

The other additional advantage is, I can expose this API:

function useObservable<T, U>(sourceFactory: (props$: Observable<U>) => Observable<T>, initialState: T, inputs: U): T

users can easily compose the props$ with their own Observable

cgross commented 5 years ago

I wouldn't expect anyone to create the Subject in component. But if they did I don't think you would switchMap or exhaustMap. Just unsub the old observable and sub to the new one. That said, I think it would be ok just not to support that like you do now with the factory. Document that its only sub-ed during mount and thats it. I don't think the case is much different with or without the factory function.

In my case, I have services that provide observables. I'm using your useObservable as a replacement for my old withObservables HOC. You can see my use case in that repo @cgross/withObservables.

And sure you'd have to provide an overloaded API like:

function useObservable<T>(source:Observable<T>,initialState:T):T;
TotooriaHyperion commented 5 years ago

The observable maybe change, then you will need to do const obs = useMemo(() => obsFromSomeWhere, [obsFromSomeWhere]);

And your use of useObservable is wrong.

You should use it in this way:

useObservable((inputObs) => inputObs.pipe(mergeMap(([obs]) => obs)),{}, [myObservable])

cgross commented 5 years ago

And your use of useObservable is wrong. You should use it in this way: useObservable((inputObs) => inputObs.pipe(mergeMap(([obs]) => obs)),{}, [myObservable])

Why ever would I do that? What does that gain vs: useObservable(() => myObservable);

TotooriaHyperion commented 5 years ago

@cgross When your observable may change. Single global immutable observable can be used as your pattern, but it's implicitly tells the hooks by a empty third parameter. Implicit may lead to bug in future maintenance. Write code that is obviously no bug is important than to do some small simplification.

cgross commented 5 years ago

@TotooriaHyperion That's horrible advice and I hope for the sake of anyone who works with you that you don't over-complicated all of your code like that.

TotooriaHyperion commented 5 years ago

@TotooriaHyperion That's horrible advice and I hope for the sake of anyone who works with you that you don't over-complicated all of your code like that.

If you read the source code, you will find that only the first output from the factory is used. Thus if your observable changes, this pattern will not work.

If you use a singleton observable, you can use

useObservable(obs) // currently not supported
or
useObservable(() => obs)

If you are using an observable that created dynamicly, you should use:

function useChangingObservable<T>(obs: Observable<T>, initial?: T): T {
  return useObservable(input => input.pipe(switchMap(([_obs]) => _obs)), initial, [obs])
}

What if the useObservable(obs) are supported at the very beginning? It will confuse people why only the first observable works. What you need is useChangingObservable.

React is not well-fit with rxjs, so we can't help but to do things ugly.

crimx commented 5 years ago

@cgross I also have issues like this and I think it's becasue the api is a bit confusing. The mixing (or splitting) of event$, inputs$ and state$ is restraining the flexibility and making the api very hard to use.

So after #60 when it finally became unusable to me. I took a step back, put some serious thoughts on the relationship between React hooks and RxJS. Ended up re-designing the api and made observable-hooks which I think fixes some of the issues including this one and #73 #51 .

Althought ended up making my own wheel, I still thank this library for bring true inspiration on hooks and observables.

leoyli commented 5 years ago

@crimx, thanks for the works!

However, wasn't the library still looks a bit too complicated?! I mean, do we really need to recreate state management over and over again?! Putting everything into observable doesn't Reacty. React already have nice solution based on useReducer and useState. And creating observable comes with memory and performance overheads. What really missed is a declarative approach to handle async user events gracefully (like in redux-observables). That is what and why I proposed #73.

I think we really need to separate the concerns here, there barely have a chance that a component will update itself without any user interaction to the UI. Those interactions are dispatched as DOMEvents, which is equivalent to actions basically. So what we need is the ability to chaining and orchestrating these events and tap UI state updates on the event streams... That is what RxJS really shines.

In other words, I don't think passing observable around or as props is a good pattern, it potentially breaks the uni-directional flow. Anyone can complete the observable silently, making it hard to debug.

crimx commented 5 years ago

However, wasn't the library still looks a bit too complicated?!

You can use the bit you need which is not complicated at all. It's like RxJS operators. There are tons but normally you just use a few of those.

I mean, do we really need to recreate state management over and over again?!

Not my intention.

And creating observable comes with memory and performance overheads.

Observables are just funciton calls which can barely impact performance in normal cases. But I agree with you on not abusing Observables.

tap UI state updates

I think tap should be avoided when possible.

I don't think passing observable around or as props is a good pattern.

@leoyli I think you misunderstood my design. props are just normal props.

crimx commented 5 years ago

Here is an example on your case

import {
  useObservableState,
  useObservableCallback,
  pluckCurrentTargetValue
} from 'observable-hooks'

const [onChange, textChange$] = useObservableCallback(pluckCurrentTargetValue)
const text = useObservableState(textChange$, '')

Or just

import {
  useObservableState,
  pluckCurrentTargetValue
} from 'observable-hooks'

const [text, onChange] = useObservableState(pluckCurrentTargetValue, '')

With deps

import {
  useObservable,
  useObservableState,
  pluckFirst
} from 'observable-hooks'

// props.prefix is normal string type
const prefix$ = useObservable(pluckFirst, [props.prefix])

const [text, onChange] = useObservableState(event$ => event$.pipe(
  withLatestFrom(prefix$),
  map(([e, prefix]) => prefix + e.currentTarget.value)
))
leoyli commented 5 years ago

@crimx, perhaps.

But as long as I see const [onChange, textChange$] = useObservableCallback, I recognize this opens the door to pass observable as props, which should be an anti-pattern. That is why I also think useObservable, in this lib too, wasn't too helpful neither.

And the pluckFirst, pluckCurrentTargetValue looks more magical. It adds extra learning curve here. If we only have useEventCallback, we solve most of use case and make it even light-weight. My idea is simply embrace what React already have, and add what is missed.

As you may find in my suggested implementation, I heavily use tap to update the state (which can also use to dispatch Redux actions). But the biggest point here is don't even expose the observable$ directly in the React component.


And I'd to learn more about why you see tap should be avoid. It definitely fit a good use case here to perform side effects. (instead just call with console.log).

crimx commented 5 years ago

@leoyli

And the pluckFirst, pluckCurrentTargetValue looks more magical.

Just simple helper functions to avoid garbage collection for common use cases. Totally optional.

My idea is simply embrace what React already have, and add what is missed.

You got your point, I repect that, and observable-hoos can also fit the tap pattern.

I think my goal for the library is more about consistency. If I already use Observables, I want to think in the Observable way. And messing with tap just break my mind flow.

leoyli commented 5 years ago

@crimx, right. I think your solutions covers the entire use case if we just want to use React + RxJS and make everything observable, which is really nice. And surely tap breaks the mind flow.

But as many experiments have been don yearly. Generally, most people will agree React doesn't fit too well with RxJS, especially when we start to optimize our app to cut down unnecessary rendering with PureComponent, memo, or useMemo. Plus, for a larger scale application, there are often used Redux (+ Redux Saga or Redux-Observable), but it is still not solve complex event management problem (imagine you are creating an image-editing app with right, left, and wheel mouse click plus keyboard combinations...).

But thank you for paying time for these interesting discussion. I'm surely would like to use your library for my next side-project.

crimx commented 5 years ago

if we just want to use React + RxJS and make everything observable

One can, but one don't have to. What I care is reusing stateful logic which is what hooks are for. With Observables now async logic can be nicely resued. This can just be a simple typeahead component.

Redux is still necessary for complex state management. But with hooks we don't have to dump everything into the global state.

crimx commented 5 years ago

But thank you for paying time for these interesting discussion. I'm surely would like to use your library for my next side-project.

Thanks! I fear the owner of this repo is going to punch me if I keep on babbling.

Brooooooklyn commented 5 years ago

@crimx feel free to make discuss happen here. rxjs-hooks is not aim to solve complicated async problems in React. We (leetcode-cn.com) already have ayanami to play the process management role. So we actually do not have many use cases in rxjs with hooks. I think the api of observable-hooks is more elegant and maybe easier to use, great works.

Austaras commented 5 years ago

@Brooooooklyn what about add another api called useSharedObservable to cover the use case of subscribe to global immutable observable?