fix: subscribe and tap type overloads

[vthinkxie]

close #6717

Description:

fix type error caused by observable subscribe function overloads

Related issue (if exists): #6717

[josepot]

I disagree with the suggested change on this PR. As a developer I much rather seeing 2 different overloads on my IDE, than conflating the already existing ones into 1 overload. Also, I don't think that #6717 is a real issue.

[vthinkxie]

I disagree with the suggested change on this PR. As a developer I much rather seeing 2 different overloads on my IDE, than conflating the already existing ones into 1 overload. Also, I don't think that #6717 is a real issue.

Hi @josepot , maybe we can discuss the issue separately.

• Should the next param here be optional? https://github.com/ReactiveX/rxjs/blob/081ca2ba7290aa3084c1477a6d4bcc573bf478f6/src/internal/Observable.ts#L78 I think this param should be optional, since the implements of the overload support optional https://github.com/ReactiveX/rxjs/blob/081ca2ba7290aa3084c1477a6d4bcc573bf478f6/src/internal/Observable.ts#L214

• Are the overloads here good overloads? According to Writing Good Overloads in typescript handbook, with the same argument count and same return type, I don't think 2 different overloads here is good overloads. Always prefer parameters with union types instead of overloads when possible.

[cartant]

This looks correct to me. And I think we'll need a similar change made to the tap signatures, too.

[vthinkxie]

This looks correct to me. And I think we'll need a similar change made to the tap signatures, too.

Hi @cartant , thanks for your comments. Should I make a change to tap in this PR or submit another PR?

[cartant]

It's so closely related that I'm fine with it being in this PR in an additional commit. Thanks.

[vthinkxie]

It's so closely related that I'm fine with it being in this PR in an additional commit. Thanks.

@cartant done

[benlesh]

I can see the spirit of this, I guess the only other thing to consider would be the documentation side of things. Both on the site and in our IDEs. Each manner of calling this has slightly different implications that should be communicated to the user. I'm sure that the single overload is probably better for TS compilation time, but what is better for the developer experience? ('m not saying I know, I'm just presenting the question: "Can we serve people better with separately documented overloads here?"

[vthinkxie]

I can see the spirit of this, I guess the only other thing to consider would be the documentation side of things. Both on the site and in our IDEs. Each manner of calling this has slightly different implications that should be communicated to the user. I'm sure that the single overload is probably better for TS compilation time, but what is better for the developer experience? ('m not saying I know, I'm just presenting the question: "Can we serve people better with separately documented overloads here?"

maybe we can add more description in the comment of source code instead of the typescript overloads?

[benlesh]

Core Team: Approval.

[vthinkxie]

Hi @benlesh any chance of merging this PR recently?

[benlesh]

Thank you, @vthinkxie!