Refine subscribe and tap signatures #4159

benlesh · 2018-09-19T16:08:21Z

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 catchError or retry-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.

The text was updated successfully, but these errors were encountered:

kwonoj · 2018-09-19T16:23:54Z

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?

benlesh · 2018-09-19T17:32:26Z

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.

benlesh · 2018-09-19T17:34:30Z

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.

kwonoj · 2018-09-19T17:38:46Z

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

ajcrites · 2018-09-19T18:09:55Z

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?

crutchcorn · 2018-09-19T23:36:22Z

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.

billba · 2018-09-20T08:33:17Z

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.

cmckni3 · 2018-09-26T04:15:13Z

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.

benlesh · 2018-09-26T17:59:54Z

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.

cartant · 2018-10-06T02:17:31Z

BTW, there is a TSLint rule in rxjs-tslint-rules - named rxjs-prefer-observer - that relates to this issue.

sebek64 · 2018-11-01T08:59:13Z

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.

cartant · 2018-11-01T09:19:43Z

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

sebek64 · 2018-11-01T09:22:03Z

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

cartant · 2018-11-01T09:26:38Z

@sebek64

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 => { /* ... */ }
    });
  }
}

sebek64 · 2018-11-01T09:38:55Z

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

cyrilletuzi · 2019-02-23T08:40:48Z

@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(() => {}, () => {}, () => {});

cyrilletuzi · 2019-02-23T08:55:53Z

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

markgoho · 2019-05-09T19:28:29Z

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!

fetis · 2019-05-24T07:36:07Z

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

gustavohenke · 2019-07-11T04:07:53Z

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

samybaxy · 2019-08-23T20:51:45Z

I'm currently on rxjs@~6.4.0
I'm passing the observer as recommended but still getting the deprecated warning on vscode.

cartant · 2019-08-23T23:08:42Z

@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 ... 🤷‍♂

JaimeStill · 2019-09-27T19:32:11Z

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

alberto-chiesa · 2019-10-21T07:51:20Z

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.

alberto-chiesa · 2019-10-21T07:56:03Z

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);

pfeigl · 2019-11-25T15:37:04Z

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

alberto-chiesa · 2019-11-25T16:53:11Z

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

* docs(subscribe): add comma after "In particular" * docs(subscribe): add notice about asynchronously thrown errors * docs(subscribe): add notice about deprecated usages of `subscribe` method (relates to ReactiveX#4159) * docs(subscribe): add notice about using `subscribe` with no params * docs(subscribe): fix example not to use deprecated `subscribe` * docs(subscribe): add example with deprecated `subscribe` with notice that it's deprecated * docs(subscribe): fix unsubscribe example replacing deprecated `subscribe` usage with Observer * docs(subscribe): add notice that unprovided error handlers will cause throwing errors asynchronously * docs(subscribe): fix return type Closes ReactiveX#5339

felixfbecker · 2020-04-25T08:42:23Z

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

cartant · 2020-04-25T08:51:23Z

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

jrista · 2020-12-07T19:59:00Z

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

digeomel · 2021-03-05T17:00:22Z

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.

rickhopkins · 2021-03-05T20:19:13Z

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.

Meigyoku-Thmn · 2021-03-11T03:54:03Z

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.

* docs(subscribe): add comma after "In particular" * docs(subscribe): add notice about asynchronously thrown errors * docs(subscribe): add notice about deprecated usages of `subscribe` method (relates to ReactiveX#4159) * docs(subscribe): add notice about using `subscribe` with no params * docs(subscribe): fix example not to use deprecated `subscribe` * docs(subscribe): add example with deprecated `subscribe` with notice that it's deprecated * docs(subscribe): fix unsubscribe example replacing deprecated `subscribe` usage with Observer * docs(subscribe): add notice that unprovided error handlers will cause throwing errors asynchronously * docs(subscribe): fix return type Closes ReactiveX#5339

* docs(subscribe): update `subscribe` documentation * docs(subscribe): add comma after "In particular" * docs(subscribe): add notice about asynchronously thrown errors * docs(subscribe): add notice about deprecated usages of `subscribe` method (relates to #4159) * docs(subscribe): add notice about using `subscribe` with no params * docs(subscribe): fix example not to use deprecated `subscribe` * docs(subscribe): add example with deprecated `subscribe` with notice that it's deprecated * docs(subscribe): fix unsubscribe example replacing deprecated `subscribe` usage with Observer * docs(subscribe): add notice that unprovided error handlers will cause throwing errors asynchronously * docs(subscribe): fix return type Closes #5339 * docs: minor grammar tweaks And remove specific mention of deprecations. Let the signature deprecations to the talking. * docs: more minor tweaks Co-authored-by: Nicholas Jamieson <nicholas@cartant.com>

benlesh · 2021-06-21T17:08:10Z

This is done and in 7.x.

florinr1 · 2023-04-19T19:16:33Z

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.

digeomel · 2023-04-19T20:43:13Z

@florinr1 if I'm not mistaken, if you follow the angular upgrade guide and use the angular cli to upgrade, these RxJs signature changes should be taken care of automatically.

benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Sep 19, 2018

kwonoj added the TS Issues and PRs related purely to TypeScript issues label Sep 19, 2018

benlesh added help wanted Issues we wouldn't mind assistance with. and removed AGENDA ITEM Flagged for discussion at core team meetings labels Sep 26, 2018

ajcrites mentioned this issue Sep 28, 2018

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

Merged

msorens mentioned this issue Aug 12, 2019

Code tightening (prepping for Angular 8) chef/automate#1212

Merged

cartant mentioned this issue Oct 23, 2019

Type signature review progress for v7 beta #5066

Closed

martinsik mentioned this issue Nov 10, 2019

refactor: RxJS chains ngx-translate/core#1137

Merged

jakovljevic-mladen mentioned this issue Mar 11, 2020

docs(subscribe): update subscribe documentation #5352

Merged

aboyton mentioned this issue Dec 7, 2020

refactor(tap): remove deprecated signatures #5917

Closed

benlesh closed this as completed Jun 21, 2021

Refine subscribe and tap signatures #4159

Refine subscribe and tap signatures #4159

Comments

benlesh commented Sep 19, 2018 • edited Loading

Proposal

Other thoughts

kwonoj commented Sep 19, 2018

benlesh commented Sep 19, 2018

benlesh commented Sep 19, 2018

kwonoj commented Sep 19, 2018

ajcrites commented Sep 19, 2018

crutchcorn commented Sep 19, 2018

billba commented Sep 20, 2018 • edited Loading

cmckni3 commented Sep 26, 2018

benlesh commented Sep 26, 2018

cartant commented Oct 6, 2018 • edited Loading

sebek64 commented Nov 1, 2018

cartant commented Nov 1, 2018

sebek64 commented Nov 1, 2018

cartant commented Nov 1, 2018

sebek64 commented Nov 1, 2018

cyrilletuzi commented Feb 23, 2019 • edited Loading

cyrilletuzi commented Feb 23, 2019 • edited Loading

markgoho commented May 9, 2019

fetis commented May 24, 2019

gustavohenke commented Jul 11, 2019

samybaxy commented Aug 23, 2019

cartant commented Aug 23, 2019 • edited Loading

JaimeStill commented Sep 27, 2019

alberto-chiesa commented Oct 21, 2019

alberto-chiesa commented Oct 21, 2019

pfeigl commented Nov 25, 2019

alberto-chiesa commented Nov 25, 2019 • edited Loading

felixfbecker commented Apr 25, 2020

cartant commented Apr 25, 2020

jrista commented Dec 7, 2020 • edited Loading

digeomel commented Mar 5, 2021

rickhopkins commented Mar 5, 2021

Meigyoku-Thmn commented Mar 11, 2021 • edited Loading

benlesh commented Jun 21, 2021

florinr1 commented Apr 19, 2023

digeomel commented Apr 19, 2023

benlesh commented Sep 19, 2018 •

edited

Loading

billba commented Sep 20, 2018 •

edited

Loading

cartant commented Oct 6, 2018 •

edited

Loading

cyrilletuzi commented Feb 23, 2019 •

edited

Loading

cyrilletuzi commented Feb 23, 2019 •

edited

Loading

cartant commented Aug 23, 2019 •

edited

Loading

alberto-chiesa commented Nov 25, 2019 •

edited

Loading

jrista commented Dec 7, 2020 •

edited

Loading

Meigyoku-Thmn commented Mar 11, 2021 •

edited

Loading