Recompose: [rx] props stream never ends

[jeron-diovis]

When ComponentFromStream is unmounted, it's obvious that props$ stream will no more receive new values – i.e., it's ended.
So, maybe, it makes sense to call observer.complete() in componentWillUnmount?
It can simplify performing cleanup on unmount, without need to use lifecycle helper and implicitly convert our functional components into classes.

Before:

compose(
   mapPropsStream(props$ => {
       ...setup some handlers and cleanup function...
       return props$.combineLatest(...);
   }),
   lifecycle({
      componentWillUnmount() {
          this.props.doSomeCleanup();
      }
   }),
   MyFunctionalComponent
)

After:

componentFromStream(props$ => {
    ...setup some handlers and cleanup function...
    props$.doOnCompleted(doSomeCleanup);
    return props$.combineLatest(..., MyFunctionalComponent);
})

[istarkov]

rxjs5 contains finally operator https://github.com/ReactiveX/rxjs/blob/master/src/operator/finally.ts
which u can use for cleanup.

As current implementation already calls unsubscribe https://github.com/acdlite/recompose/blob/master/src/packages/recompose/componentFromStream.js#L49 so complete will be called

[jeron-diovis]

Ok, finally.

Unsubscribing causes it to be called? Can you explain? (I don't know spec)
Because now I'm using recompose with Kefir observables, and it's onEnd callbacks are not called when component is unmounted.

[jeron-diovis]

Also,

which u can use for cleanup

How can I get access to that observer? It is only available here. I have only props$ stream, nothing more.

[istarkov]

https://jsfiddle.net/2j2esxyL/
Click Off button, see Done in console.

[jeron-diovis]

Hmmm, ok, thanks for example.

Probably it's implementation details of particular lib. https://jsfiddle.net/2j2esxyL/2/ – no message in console on clicking "OFF".

[umidbekk]

@istarkov

rxjs5 contains finally operator

But .finally does not pass last stream value

const enhancer = compose(
  connect((state) => ({ count: state }), {increment, decrement, cleanup}),
  mapPropsStream((propsStream) => {
    const cWUStream = propsStream
        .finally((props) => {
            // props === undefined
            props.cleanup()
        })
        .last()
        .do((props) =>{
            // never calls
            props.cleanup()
        })
        .startWith(null)

    return propsStream.combineLates(cWUStream, (props) => props)
  })
)

[wuct]

I agree we should call observer.complete() in componentWillUnmount. To achieve this feature, maybe we can bind the observer to the class properties and call it directly, instead of passing props via the change-emitter. Something like:

  class ComponentFromStream extends Component {
    state = { vdom: null };

    observer = null

    props$ = config.fromESObservable({
      subscribe: observer => {
        this.observer = observer 
        return { unsubscribe: this.observer = null }
      },
      [$$observable]() {
        return this
      }
    });

    // Stream of vdom
    vdom$ = config.toESObservable(propsToVdom(this.props$));

    componentWillMount() {
      this.subscription = this.vdom$.subscribe({
        next: vdom => {
          this.setState({ vdom })
        }
      })
      this.observer.next(this.props)
    }

    componentWillReceiveProps(nextProps) {
      this.observer.next(nextProps)
    }

    shouldComponentUpdate(nextProps, nextState) {
      return nextState.vdom !== this.state.vdom
    }

    componentWillUnmount() {
      this.observer.complete(this.props) // call `complete`
    }

    render() {
      return this.state.vdom
    }
  }

It's just a quick solution comes into my mind. @istarkov how do you think?

[istarkov]

Looks like calling complete is a good idea.
As props$ observable is really completed.

[wuct]

Yup, it's a good practice to call complete when a stream is completed.

[umidbekk]

@wuct componentFromStream needs change-emitter to maintain multiple subscriptions to props stream, eg.:

 mapPropsStream((props$) => {
   const a$ = props$.pluck('a')
   const b$ = props$.pluck('b')

   return props$.combineLatest($a, $b, (props, a, b) => ({...props, c: a + b}))
})

Here add an extra argument to .emit to solve this problem

[istarkov]

no need in second argument, checking that props is not an object is enough. so emitting without arguments will be complete call

[wuct]

@umidbekkarimov You are right. Thanks for correcting me.