Track active observables

[bpinto]

Hey all, first of all thanks for creating and managing this library.

I'd like to discuss adding support for tracking active observables so that we may support features like this on tests:

import { activeObservables, clearActiveObservables } from 'kefir'

beforeEach(() => { clearActiveObservables() })
afterEach(() => { expect(activeObservables.length).toEqual(0) })

and to eventually create a browser extension or even simpler to do a $K.activeObservables on the browser console - assuming you've bound kefir to window window.$K = Kefir - and see if there are any memory leaks on your code.

• [x] docker support https://github.com/kefirjs/kefir/pull/316
• [x] upgrade mocha https://github.com/kefirjs/kefir/pull/318
• [x] npm run watch https://github.com/kefirjs/kefir/pull/319
• [x] track active observables on dev build only
• [x] update tests that were not unsubscribing from all observables

P.S.: I've pushed two commits labeled "remove me" with the intention to diff the modified dist files (see second commit), these commits are going to be removed before merging this PR.

[mAAdhaTTah]

Definitely going to need to ponder this a bit.

I'm not confident on my Proxy usage. Perhaps someone has another idea on how to track observables?

I don't think we need to go full Proxy. You can just monkeypatch the method. If you wanted to use Proxy, you should probably Proxy the entire Observable object and then trap the particular methods in the Proxy handlers.

[bpinto]

You can just monkeypatch the method.

What is your suggestion so I could try implementing it? One problem I had before was that Kefir.atom and other classes will extend this Observable class and thus remove any implementation we set on _onActivation and _onDeactivation.

[mAAdhaTTah]

What is your suggestion so I could try implementing it?

Ultimately not terribly different from what you have, except instead of returning a proxy, you return a new function.

One problem I had before was that Kefir.atom and other classes will extend this Observable class and thus remove any implementation we set on _onActivation and _onDeactivation.

Not 100% sure I follow; if you have an example, that would be helpful. But if we're wrapping the method, because we still end up calling the underlying method, I don't think it'll be an issue.

[bpinto]

Ultimately not terribly different from what you have, except instead of returning a proxy, you return a new function.

Ok, I'll update this PR soon.

Not 100% sure I follow; if you have an example, that would be helpful. But if we're wrapping the method, because we still end up calling the underlying method, I don't think it'll be an issue.

I misunderstood what you said earlier, I thought you had suggested we wouldn't call the original method. 👍

[bpinto]

Okay, so I've pushed another code which does not use Proxy, now there is something weird happening, the following test would not unsubscribe the observable:

export const getValue = maybeObservable => {
  if (!isObservable(maybeObservable)) return maybeObservable
  let value
  const setValue = val => value = val
  const firstValue = maybeObservable.take(1)
  firstValue.onValue(setValue)
  firstValue.offValue(setValue)
  return value
}

test('returns syncronous value from observables', () => {
  expect(streams.getValue(K.constant(1))).toBe(1)
})

The following observable is still tracked by Kefir.activeObservables:

    [ AnonymousObservable {
        _dispatcher: null,
        _active: false,
        _alive: false,
        _activating: false,
        _logHandlers: null,
        _spyHandlers: null,
        _onActivation: [Function: _onActivation],
        _onDeactivation: [Function: _onDeactivation],
        _currentEvent: { type: 'value', value: 1 },
        _source: null,
        _name: 'constant.take',
        _n: 0,
        '_$handleAny': null } ]

I don't understand yet why this observable has not been removed from the array, but I'll debug it further.

P.S.: Would you rather use apply from src/utils/function ?

[bpinto]

@mAAdhaTTah I've finally come back to this PR and while there are commits that I need to extract into smaller PRs (some I already have), this is ready for review.

I have a commit where I had to brute-force subscription deactivation because unless I missed it, I don't think there is another way to handle it with the existing code.

I'm not sure if it's worth exporting the onAny function on toPromise as the subscription should be deactivating automatically as a promise always ends, right?

[bpinto]

@mAAdhaTTah I'm thinking that maybe we shouldn't introduce a new dev dist and just include these methods on the normal distributed files?

[mAAdhaTTah]

Maybe I should ask more generally: what's the purpose of this? Why do you need to track active observables?

[bpinto]

I think the main purpose of this would be to track memory leaks (or observable leaks).

An usage example:

afterAll(() => {
  if (activeObservables.length !== 0) {
    fail()
  }
})

test('all observables are unsubscribed', () => {
  const {unmount} = render(<MyComponent />)
  unmount()
})

It's very easy to make a mistake and forget to unsubscribe an observable when using .observe for instance. To confirm how easy it is to introduce these memory leaks, I fixed some kefir tests that had not unsubscribed to all observables.

[mAAdhaTTah]

If the primary goal is testing, I think something like this might be more suited to the kefit-test-utils package. We could monkey patch the Observable.prototype in that package to track this info without expanding the base package for an uncommon use case.

[bpinto]

We could, though I think that tracking observables during a test environment is not the only usage. We could for instance want to run this on our browser window and make sure that while navigating from one page to another the number of observables is corresponding to what we expect.

I was thinking about using a configurable environment variable that would default to the test environment so that users could opt in and out in any environment.

const { NODE_ENV, TRACK_OBSERVABLES }
const trackObservables = TRACK_OBSERVABLES || NODE_ENV === 'test'

P.S.: I'm happy to move it to kefir-test-utils if you think it's not worth adding it to kefir.