Refine subscribe and tap signatures
benlesh opened this issue · 36 comments
Currently we have a variety of ways you can use subscribe and tap:
o.subscribe(fn, fn, fn);
o.subscribe(fn, fn);
o.subscribe(fn);
o.subscribe(void, void, fn);
o.subscribe(void, fn, fn);
o.subscribe(void, fn);
o.subscribe(observer);
o.pipe(tap(fn, fn, fn));
o.pipe(tap(fn, fn));
o.pipe(tap(fn));
o.pipe(tap(void, void, fn));
o.pipe(tap(void, fn, fn));
o.pipe(tap(void, fn));
o.pipe(tap(observer));It's been my personnel recommendation that people stick to the following two signatures:
o.subscribe(fn);
o.subscribe(observer);
// or likewise with tap:
o.pipe(tap(fn));
o.pipe(tap(observer));The reason being is:
// POOR readability (I have to count arguments?)
o.subscribe(null, null, () => {
// side effect here
});
// BETTER readability
o.subscribe({
complete() {
// side effect here
}
});
// GOOD readability
o.subscribe(x => {
// side effect here
});
// GOOD readability (if unnecessarily verbose)
o.subscribe({
next(x) {
// side effect here
}
});Proposal
I'd like to deprecate the other signatures to subscribe and tap, and narrow down the signature to just those two prescribed above for the next major version
Pros
- Simplify subscription code slightly.
- Simpler typing for TypeScript.
- Guides people into writing cleaner, more readable code
Cons
- The behavior around handling errors at the point of subscription may get a slightly harder to control, with only one way of adding error handling at subscription time. (NOTE: errors should really be handled by a
catchErrororretry-type operator, rather than in the subscribe.)
Other thoughts
It would also be good to add some lint rules around this to rxjs-tslint and the like.
I think we can have tslint rules easily, that's probably not a problem.
For librarywise, I'd like to think bit more: specifically, you'd like to allow form of
subscribe(next);
but do not allow any other forms? or it is still allowed
subscribe(next, error);
subscribe(next, error, complete);
form?
but do not allow any other forms? or it is still allowed
It would be allowed in the short term, but in the long term (next major version or some future major version), it would be a hard API change.
And if anyone is wondering "why not just subscribe(observer) and forEach(fn)?"... Well, forEach is supposed to have the semantic of nexting on a microtask, and forEach returns a promise and is currently non-cancellable.
I'd expect that .subscribe(fn) is probably the 90% use case for people.
I'd expect that .subscribe(fn) is probably the 90% use case for people.
I agree it's major usecase, but not sure about 90%. i.e these two form
subscribe(next, error);
subscribe(next, error, complete);
I've been used all time, personally I never used observer object form (.subscribe({next: ...})).
I don't have strong opinion around readability between observer form vs. fn arguments, but does not allowing fn form except next bitters me, bit downvote around those decisions.
what if we allow fn form as is, only disallow optional form?
subscribe(null, error); //meh, not allowed
subscribe(next, error); //allowed
I'm all for this change; in fact I've recently started doing subscribe({ error }) instead of subscribe(null, error) simply because I found out that you could even do the former. I think that this deprecation will coach people into what I see as a better way of writing subscription code.
I'd like to discuss the "Con" a bit more:
The behavior around handling errors at the point of subscription may get a slightly harder to control...
Do you have more specifics about this? It seems like .subscribe({ error }) is not harder than .subscribe(null, error), but I'm not sure if I'm missing something.
...errors should really be handled by a catchError or retry-type operator, rather than in the subscribe.
I don't fully agree with this for a couple of reasons:
- We need a way to handle errors that happen in
next. - This requires a particular way of thinking about Observables -- specifically that everything flows through to the subscription handler and caught errors need to emit something that can be handled by that logic. Are you suggesting that we always want that to be the case?
I honestly could not agree more. With our work with RxJS, we've had so many bike shedding discussions due to this issue and the students I've helped teach RxJS often get caught on this issue (of error handling/syntax) based on the numerous ways to approach things and the different (usually 3rd party) resources.
What I specifically like about observable.subscribe(next, error) is that it's a nice parallel to promise.then(result, error). Although since the semantics are different, maybe that's an argument for getting rid of this "faux amis" situation. Okay, I've convinced myself that I agree with this proposal.
I agree with this. I found having the next, error, and complete handlers becomes difficult to read. Moved to pipe approach for everything with v5 and v6.
From the core team meeting:
Start with deprecating signatures like null, null, fn. Perhaps add some tslint rules to help guide people. Maybe move on to deprecating fn, fn, fn... @kwonoj is the voice of dissent.
BTW, there is a TSLint rule in rxjs-tslint-rules - named rxjs-prefer-observer - that relates to this issue.
Although I agree that observer syntax is a bit more readable, it is not always a win in practice. Consider the following code:
class A {
private status = false;
private subscription: Subscription | null = null;
constructor(private readonly obs: Observable<any>) {}
someMethod() {
this.subscription = this.obs.subscribe(value => {
// notice the usage of this here
this.status = true;
}, err => {
// some error handling
});
}
}When attempting to transform this code to observer syntax, it fails on unavailability of "this".
Moreover, readability in a good editor (like IntelliJ Idea) is better, because the IDE shows parameter names. It looks like this:
class A {
private status = false;
private subscription: Subscription | null = null;
constructor(private readonly obs: Observable<any>) {}
someMethod() {
this.subscription = this.obs.subscribe(next: value => {
// notice the usage of this here
this.status = true;
}, error: err => {
// some error handling
});
}
}I would appreciate any suggestions how to transform this code to observer syntax with retaining availability of "this". If that is not easily possible, I'd vote for un-deprecating these methods.
@sebek64 The only difference between your second snippet and the observer syntax is two enclosing braces. Well, that and that fact that your snippet isn't valid TypeScript.
@cartant Yes, it isn't a valid typescript. It is how the code is displayed by Idea. The added argument names "next" and "error" and in a different font (smaller, grey).
class A {
private status = false;
private subscription: Subscription | null = null;
constructor(private readonly obs: Observable<any>) {}
someMethod() {
this.subscription = this.obs.subscribe({
next: value => this.status = true,
error: err => { /* ... */ }
});
}
}@cartant Thanks, this works. I haven't realized that this syntax is possible in Typescript. So in this case it is a good idea to keep the deprecations :)
@benlesh Do we agree this is still OK in v6.4 and further?
someObservable.subscribe(() => {});Because when doing that, TypeScript IntelliSense is showing a signature flagged as deprecated since v6.4...
And it's not clear if the following are still OK or not:
someObservable.subscribe(() => {}, () => {});someObservable.subscribe(() => {}, () => {}, () => {});After some investigation, the deprecated descriptions are shown by IntelliSense in all cases!
TypeScript doesn't support to have different JSDoc for different overloads. TypeScript will aggregate all the JSDoc available for the function. It creates a very confusing situation...
Here's what you get when you hover on .subscribe() in VS Code:
I know this is a WIP but without a reasonable investigation, it looks like everything that you might pass in to subscribe has been deprecated!
I get deprecate warning on a quite valid, imo, construction. I'm on rxjs@6.5.2
So, is this syntax is still allowed or not? The original PR doesn't have intention to change this.
I use WebStorm
I like the change that was done in #4202 to support this discussion.
However, it seems that now observables that I convert from other libs (e.g. mobx-utils, see simplistic mobxjs/mobx-utils#138) don't compile.
It seems that they now must have a subscribe overload that supports all those?
Argument of type 'IObservableStream<number>' is not assignable to parameter of type 'ObservableInput<any>'.
Type 'IObservableStream<number>' is not assignable to type 'Subscribable<any>'.
Types of property 'subscribe' are incompatible.
Type '{ (observer: IStreamObserver<number>): ISubscription; (observer: (value: number) => void): ISubscription; }' is not assignable to type '{ (observer?: NextObserver<any> | ErrorObserver<any> | CompletionObserver<any> | undefined): Unsubscribable; (next: null | undefined, error: null | undefined, complete: () => void): Unsubscribable; (next: null | undefined, error: (error: any) => void, complete?: (() => void) | undefined): Unsubscribable; (next: (val...'.
Types of parameters 'observer' and 'next' are incompatible.
Type 'null | undefined' is not assignable to type 'IStreamObserver<number>'.
Type 'undefined' is not assignable to type 'IStreamObserver<number>'.
I'm currently on rxjs@~6.4.0
I'm passing the observer as recommended but still getting the deprecated warning on vscode.
@samybaxy That's a bug/feature of the pop-up window that's shown in the editor. The summary of the deprecated messages is shown for all usages of subscribe. Whether or not a linter reports the usage as deprecated is a better indication. There's nothing we can do about this. Some signatures are deprecated and some aren't. And some tooling cannot cope with that so ... 🤷♂
Along with @fetis I would concur that .subscribe(fn, fn) is a valid signature. In Angular services, all of my HTTP calls are structured with a subscribe using this signature. i.e.:
this.http.get<Item[]>(`/api/item/getItems`)
.subscribe(
data => this.items.next(data),
err => this.snacker.sendErrorMessage(err.error)
)I wonder why anyone didn't stop for a moment wondering if deprecating working code in large codebases was such a great idea.
The signature with 3 callbacks is a natural extension of the Promise A then, and comes natural for anyone accustomed to that.
(1) Optional linting rules for teaching, (2) exposing more of the "recommended" syntax, (3) producing an automated tool to mechanically convert from one version to the recommended one: those are fine.
Breaking the build for medium or large (50k lines of code) codebases WITHOUT any functional advantage IS NOT GOOD, people.
Please, please stop breaking my code at any upgrade. And don't tell me that a major is breaking by definition and everything is fine, because it's not: hundreds of deprecation warnings, just for the sake of it, in my build are definitely not fine.
And, regarding the initial post: you're inflating numbers. Mnemonically we have only two signatures:
o.subscribe(fn | null, fn | null, fn | null);
o.subscribe(observer);@alberto-chiesa Thankfully we are not the only ones wondering about those changes.
Question towards the the collaborators: Will there possibly atleast be schematics to fix those deprecations when the code is actually removed (I would have loved to have the schematics along with the deprecation).
@pfeigl I think no one is caring enough about the sanity of existing developers using this library. A breaking change such as pipe has many technical reasons in order to justify the breaking of existing code, but having this kind of massive deprecation notices spreads confusion between teammates and people being onboarded in RxJS (which has a steep learning curve, anyway).
API size is a burden, ok, but API deprecation is far worse: "it's more readable IMO" is NOT ENOUGH.
I love RxJS, I'm an absolute fan of it, but keep in mind that enterprise environments are a thing!
If I use Angular and they are keeping RxJS updated (as they should), and I want to keep Angular updated (as everyone should), I need to be able to update it in the simplest way possible.
Please, please, please, keep it stable. If the only advantage is "readability" and it can break something, the trade off should NOT be considered. You carry the responsibility of thousands of codebases. Readability is important, but it should not break the build.
Could we create an ESLint rules with an autofixer that converts the deprecated signatures to the object signature? This could serve as an automatic migration script to make upgrading painless (and hopefully aid in adapting before the new version is even out). The intermediate state with all these deprecation notices and many overloads definitely has a cost.
cc @cartant who has the excellent https://github.com/cartant/eslint-plugin-rxjs
@felixfbecker There are a bunch of deprecations that I think need reconsideration - the subscribe signature is one of them. They can be discussed as part of the signature/types review. IMO, v7 should not go out until the deprecations have been cleaned up, the messages have been made consistent and URLs to explanations are included in the messages.
@benlesh I've just read your issue itself....and in all honesty, I frequently use these signatures myself:
o.subscribe(fn, fn, fn);
o.subscribe(fn, fn);
o.subscribe(fn);
o.subscribe(observer);
o.pipe(tap(fn, fn, fn));
o.pipe(tap(fn, fn));
o.pipe(tap(fn));
o.pipe(tap(observer));
I've never used the versions with void...not sure if that means you could just pass void, or it just means passing () => {}. In any case...there is a lot of code in a lot of my code bases that use all of the above signatures.
My honest opinion here is, if all but the last two cases break if I upgrade to RxJs 7, I'll be delaying that upgrade for quite some time. I understand the desire to simplify...but something I feel very strongly about is that the javascript community is far too disinterested in backwards compatibility, and in the long run that puts a lot of burden on developers, on companies...on our clients funds...do deal with the never-ending train of breaking changes.
As an example, I endured quite a lot of pain across many projects to deal with the Ionic 3 -> Ionic 4 upgrades...which was a very poorly supported upgrade path. The cost burden to deal with it went to my clients, of course...but it was a frustrating experience each time for myself and other devs as well. I would hate to see RxJs 7 impose the same kind of frustration.
I think we should be cognizant of that when making decisions that will force upgraders to endure a lot of pain in the process. I'm also an avid gamer, and I'm definitely in the camp of gamers who staunchly support delaying the game release, if it means getting the game right, with as few bugs as possible, on release. I feel the same way about libraries like RxJs...I would much rather time be spent to make the RxJS 7 release a smooth one that doesn't put a lot of pain on the developer, or drain on our clients budgets, to deal with tons of breaking changes, or migration issues, etc. than to have it rushed out before the end of the year...just to get it out before the end of the year.
My honest thoughts (I suspect I'll be sharing more...this is the first of the checklist I've taken a look at.)
Hello, in the vast majority of our code, we use the signature subscribe(fn). If I understood well from the above, this is still a valid (i.e. not deprecated) syntax. But I just upgraded vscode to the latest (1.54.1) and suddenly all our subscribe() methods are triggering the warning:
subscribe is deprecated: Use an observer instead of a complete callback (deprecation)tslint(1)
This is an Angular 11 project with rxjs 6.5.4. ng lint doesn't trigger those warnings. So is it a problem with vscode? I even set the option "editor.showDeprecated": false and it seems to have no effect.
I believe this is a typescript issue. Something in the newest versions of typescript is causing this warning to display in vs code. I was able to get it to go away by click the version of typescript in the bottom right corner of vs code and then choosing the select typescript version option. I set it to the node_modules version we have installed in our angular project which in our case happens to be 4.0.7. This caused the warnings to go away.
It seems that a new Typescript SDK version (or a combination of a specific Typescript version with a specific TSLint plugin version) causes this. A project that I'm working on has this exact problem even though it's dependencies haven't been updated for months. Set the Typescript SDK to the older version in node_modules works for me.
This is done and in 7.x.
I use RxJS in an enterprise large Angular codebase and I've just upgraded Angular to 15.2 and RxJS to 7.8. A simple search for .subscribe in my main project returns 1185 results. I'm seeing a lot of deprecated .subscribe method calls and to be honest, it's frustrating.
The deprecated warnings are not an issue for now but if you are planning on removing the deprecated subscribe implementations in the next release there will be a huge problem for me. It's absurd to spend a couple of days to change all the .subscribe methods only for the sake of readability. As a core library I expect RxJS to be able to withstand the test of time and not deprecate functions just because they look ugly or "Simplify subscription code slightly.".
Please reconsider removing the deprecated methods in the next releases.