acdlite / recompose

A React utility belt for function components and higher-order components.
MIT License
14.75k stars 1.26k forks source link

memory leak of mapPropsStream at server side #328

Open istarkov opened 7 years ago

istarkov commented 7 years ago

Seems like current mapPropsStream causes a memory leak at the server (server side rendering), and having that the idea to call subscribe at componentDidMount is not the thing what I want in most cases, the workaround I see is to use at server side observable config like

const config = {
  fromESObservable: Rx.Observable.from,
  toESObservable: stream => stream.take(1) // take(1) is a solution
}

As an example how take will help https://jsfiddle.net/2j2esxyL/7/

So having that there will be no call of componentWillUnmount at server side, take(1) will close the stream so there will be no memory leaks and all event listeners will be removed.

Even the solution above will not solve all the cases, (the case at which mapPropsStream will wait something inside is not covered) It seems like it will work for all cases I have internally.

I think if it's true (I haven't checked problem above) we need to add this into documentation.

@wuct ^

wuct commented 7 years ago

Found a related issue https://github.com/facebook/react/issues/3714

wuct commented 7 years ago

It seems there is not to much we can do without detecting the runtime environment.

mapPropsStream will wait something inside

I am not sure what do you mean. Can you share an example?

wuct commented 7 years ago

Maybe we can make two subscriptions, one in componentWillMount and one in componentDidMount. The one in componentWillMont will unsubscribe immediately after setState.

    componentWillMount() {
      const subscription = this.vdom$.subscribe({
        next: vdom => {
          this.setState({ vdom }, subscription.unsubscribe)
        }
      })
      this.propsEmitter.emit(this.props)
    }

    componentDidMount() {
      this.subscription = this.vdom$.subscribe({
        next: vdom => {
          this.setState({ vdom })
        }
      })
    }

The one in componentDidMount just works like how it should work and will be unsubscribed in componentWillUnmount.

istarkov commented 7 years ago

Immediate unsubscription will cause cancellation of user created streams, in all cases its not an expected behavior. Will write an example of situation at which take(1) will not solve a problem later, now on mobile.

avocadowastaken commented 7 years ago

I guess stream.take(1) is only good solution for SSR now. We need serverDidRender hook (can't find link to discussion).

wuct commented 7 years ago

Agree. I can't come up with other solutions.