Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Oct 10, 2018

Conversation

ajcrites
Copy link
Contributor

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
Copy link

coveralls commented Sep 28, 2018

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 Coverage Status
Change from base Build 7290: -0.2%
Covered Lines: 5244
Relevant Lines: 5417

💛 - Coveralls

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the initiative on this.

@@ -70,6 +70,12 @@ export class Observable<T> implements Subscribable<T> {
}

subscribe(observer?: PartialObserver<T>): Subscription;
/** @deprecated */
subscribe(next: null, error: null, complete: () => void): Subscription;
Copy link
Collaborator

@cartant cartant Sep 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ajcrites
Copy link
Contributor Author

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
Copy link
Collaborator

cartant commented Sep 28, 2018

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
Copy link
Collaborator

cartant commented Sep 28, 2018

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
Copy link
Member

kwonoj commented Sep 28, 2018

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
Copy link
Collaborator

cartant commented Sep 28, 2018

@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
Copy link
Member

kwonoj commented Sep 28, 2018

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
Copy link
Collaborator

cartant commented Sep 28, 2018

@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
Copy link
Contributor Author

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 cartant closed this Sep 29, 2018
@cartant cartant reopened this Sep 29, 2018
@cartant
Copy link
Collaborator

cartant commented Sep 29, 2018

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
Copy link
Collaborator

cartant commented Sep 29, 2018

@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.

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@benlesh
Copy link
Member

benlesh commented Oct 10, 2018

@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).

@benlesh benlesh merged commit c85ddf6 into ReactiveX:master Oct 10, 2018
@ajcrites ajcrites deleted the deprecate-subscribe-fn branch October 10, 2018 23:34
@lock lock bot locked as resolved and limited conversation to collaborators Nov 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants