Add CompositeDisposable::addIfDisposable

[as-cii]

This should allow package developers to catch issues like atom/tabs#183 early on.

This method will allow to have less strict checks whenever we add a disposable for code that we do not manage, thereby avoiding issues like atom/tabs#183 altogether.

/cc: @kevinsawicki @benogle

[mnquintana]

🎨 What about rejects an object that does not implement ::dispose?

[as-cii]

It’s probably better but eventually I went with throws an error, as it’s more consistent with how specs are named in atom/atom. Thanks! 🙇

[kevinsawicki]

Since disposable.dispose could be altered after it is added to the disposables array, I think this case should be explicitly guarded against during CompositeDisposable::dispose as well.

[kevinsawicki]

Also, do you know if this would cause the git-log package to now be associated with the error notification instead of the tabs package for the referenced issue case?

[as-cii]

Since disposable.dispose could be altered after it is added to the disposables array, I think this case should be explicitly guarded against during CompositeDisposable::dispose as well.

Nice catch, I have just added it. 👍

Also, do you know if this would cause the git-log package to now be associated with the error notification instead of the tabs package for the referenced issue case?

I think the error will still be raised on tabs in that particular case but in a different moment and with a more insightful error message. The matter with that package is that some functions are implemented without respecting “the contract” between TabView and its @item.

Ideally, a type system would warn us about such kinds of issues but given that there isn't one, I think this is best thing we could possibly do. Also, I wouldn’t silently skip null disposables, as I see it as a potential root of problems.

@kevinsawicki: thoughts?

[kevinsawicki]

@kevinsawicki: thoughts?

Yeah, thinking about the tabs example, it would seem unfortunate to have to wrap calls to add and dispose now in a try/catch since showing a notification isn't helpful at all and associating the pane item to a package is difficult/impossible.

Having CompositeDisposable throw exceptions makes feature degradation based on expected APIs not returning disposables rather challenging to work around.

I feel like the pattern will be people just wrapping these methods in try/catch blocks that do nothing and just moving on.

[as-cii]

The problem this PR is trying to address comes from a package not respecting an API’s implementation contract. In particular, atom/tabs#183 was caused by the package implementing a function (which was supposed to return a Disposable) by returning null instead.

Ideally, a type system would catch these kinds of errors early on thereby solving the problem altogether. However, since we haven’t got such facility in our environment, how can we solve it?

This PR was initially created to put stricter guards whenever we added new disposables to a CompositeDisposable. At first, I was convinced that we needed to raise an error as early as possible, so that package authors could have better insights on what to do when things failed.

After discussing about this with @kevinsawicki, though, we agreed that there could have been situations where package authors simply ignored these new exceptions, thus leaving the problem unsolved. Therefore we decided to introduce a new API (addIfDisposable) that simply ignores objects that do not respect the Disposable contract; this should be used when dealing with code that we do not manage, such as the one that caused the exception in the aforementioned issue. add, on the other hand, should be kept for scenarios where we want stricter checks, such as core code.

@benogle @maxbrunsfeld @nathansobo: what do you think?

[nathansobo]

I feel a bit weirded out about expanding the API of what is otherwise a super minimal library. How many places do we anticipate using this method? Would it be rare enough to just guard the value in those cases and go ahead and throw exceptions from the core library?

[maxbrunsfeld]

I still think CompositeDisposable::add should throw an exception if the argument doesn't implement .dispose(). In most cases, something's wrong if we're trying to do that, and It'll often be a memory leak. I don't think we'd ever want to purposefully catch that exception, since it's a programmer mistake, not a runtime error condition.

I think that if we went that route, we should have TabView check the return value of ::onDidChangeTitle and friends, and log a warning to the console if they don't return disposables. Maybe we should add a Disposable.isDisposable() to clarify what we're doing when we check.

[nathansobo]

Maybe we should add a Disposable.isDisposable() to clarify what we're doing when we check.

👍

[as-cii]

Closing in favor of #15. Thanks everyone for your feedback! 🙇