fix(subscribe): Deprecate null starting parameter signatures for subscribe

[ajcrites]

Description:
Per discussion on #4159, this deprecates the signatures for .subscribe and .tap that are (null, null, complete), (null, error, complete), and (next, null, complete).

Related issue (if exists):
#4159

[coveralls]

Pull Request Test Coverage Report for Build 7307

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.2%) to 96.806%

---

Totals
Change from base Build 7290:	-0.2%
Covered Lines:	5244
Relevant Lines:	5417

---

💛 - Coveralls

[cartant]

Thanks for taking the initiative on this.

[cartant]

I think undefined would be the way to go - rather than null.

At the moment, it's not possible to call subscribe and pass null as an argument without TypeScript complaining, so unless the existing, non-deprecated signatures are altered to accept null arguments, I think the deprecations will be largely ineffectual.

[cartant]

There'd be no harm in using null | undefined, though.

[ajcrites]

By the way, as part of my additional fixup for this it may make sense to have a specific deprecation message. Do we have copy in mind that we want to add?

[cartant]

I'm actually thinking about that right now, as I'm writing a TSLint rule to do the same. I'll let you know what I come up with.

[cartant]

Regarding the coveralls failure. It seems to report +/-0.2% changes in a seemingly non-deterministic way. I was speaking to someone on Monday, at a meetup, and he was of the opinion that it was due to the PR's branch not being completely up-to-date with master.

As an experiment, could you ensure your master is up-to-date and then rebase your PR and push it again? Like this:

git checkout master
git fetch upstream
git rebase upstream/master
git checkout deprecate-subscribe-fn
git rebase master
git push -f

I'd be really interested in whether or not coveralls passes after that. This +/-0.2% variation has been an annoyance for a long time.

[kwonoj]

regarding above, I'm curious if generated coverage have problem or the way coveralls translate it have some problems. ..brutal way, maybe we can setup codecov also and 1:1 compare for a while per each PRs?

[cartant]

@kwonoj I don't really know enough about Coveralls or what it's doing. I mentioned the problem during a talk (on contributing to OSS) and someone came up to tell me about his experiences, afterwards.

If we can establish what the problem is, we might be able to get a decent solution. ATM, if you cannot reliably see whether the coverage goes up or down, it's not especially useful.

TBH, though, I'm not super keen on getting into the coveralls nitty gritty. 😬

[kwonoj]

I am usually just ignoring those minor deets, so on same boat. Only reason I care is most new contributor got highly confused like 'I only changed doc and coverage dropped, maybe I did something wrong'.

[cartant]

@ajcrites I'd suggest something like:

/** @deprecated Use an observer instead of an error callback */

And would tailor the message to whichever function the deprecation applies.

[ajcrites]

We're in the midst of updating the terminology "observer" to "subscriber," at least in some documentation. Should I stick with "observer" for this?

[cartant]

Sorry about the closure. On mobile with fat fingers.

My understanding is that the documentation is changing re: new Observable(subscriber => {}) - where the parameter really is a Subscriber - and that's not the case here. The argument is not a subscriber. I could be wrong, but that's my take. In fact, the type that corresponds to the argument, here, is actually PartialObserver, as not all methods are required.

[cartant]

@ajcrites BTW, you can disregard my request to experiment with rebasing your PR. I've grabbed a copy of your branch and I'm going to do some experimentation of my own.

[cartant]

LGTM

[benlesh]

@ajcrites ... subscribe accepts a PartialObserver, which is any subset of Observer... Subscriber is a superset of Observer in that it has a closed property on it (as well as other things in RxJS 5 and 6).