adding onDispose

[tsouza]

The rxjs Observable constructor allows passing a subscribe function that returns a TeardowLogic, which is a function that is called when subscription is cancelled. https://rxjs.dev/api/index/class/Observable#constructor

This PR adds a onDispose named parameter to the create constructor with the same purpose.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (079d6d6) 99.36% compared to head (4dc855d) 99.36%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #17   +/-   ##
=======================================
  Coverage   99.36%   99.36%           
=======================================
  Files         110      110           
  Lines        1737     1739    +2     
=======================================
+ Hits         1726     1728    +2     
  Misses         11       11           

Impacted Files	Coverage Δ
lib/src/constructors/create.dart	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[renggli]

Hmm, I don't think the separate dispose callback is facilitating a good (or even useful) coding practice. The creation callback can be called multiple times, and it is not clear how the dispose callback can then clean-up the correct corresponding state?

I recommend that you create disposables during the creation callback:

final observable = create<String>((emitter) {
        emitter.next('a');
        /* ... */
        emitter.add(ActionDisposable(() => /* free expensive resource */));
      });

Maybe there are some ways to make this code a bit more obvious?

[tsouza]

Maybe there are some ways to make this code a bit more obvious?

I agree with your new implementation, but maybe we should keep onDispose named parameter as to try to match closer to rxjs API?

[tsouza]

(Quoting myself)

but maybe we should keep onDispose named parameter as to try to match closer to rxjs API?

I see that there is a significant difference between exposing an onDispose parameter compared to the current supported way (what you have suggested) and rxjs's way.

Initially implemented this by changing the signature of the callback on the create function to return something else other than void, but I am not sure about this design too.

• If returning something like OnDispose?, would not only break current API but also require every callback implementation to return null.
• If returning dynamic, then can not statically check.

So the solution:

emitter.add(ActionDisposable(() => /* free expensive resource */));

Has the least disadvantages aside of not being obvious. So maybe document it or add some:

emitter.doOnDispose(() => /* free expensive resource */);

[renggli]

Surprised that emitter.add is not that well know, since it is also supported by RxJS. Expanding the documentation on the create method could help here, but I am hesitant to add new API.

Making the callback accept a dynamic return value as in Map1<Subscriber<T>, dynamic /* Disposable|Callback0|null */> is also a reasonable option. That would be the closest to the API of RxJS, but potentially a source of bugs? My hope is that Dart will eventually support Union types.

[tsouza]

Let's follow that option then (I've pushed it). It may open doors for bugs, but other solutions has issues too and this one at least makes ground for when/if union types gets implemented.

[renggli]

Unfortunately this breaks some of my code in very subtle ways, because the creation method happens to return something unrelated :-(