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

Amendment to mapTo operator type #5090

Closed
alaboudi opened this issue Oct 17, 2019 · 5 comments
Closed

Amendment to mapTo operator type #5090

alaboudi opened this issue Oct 17, 2019 · 5 comments
Labels
TS Issues and PRs related purely to TypeScript issues

Comments

@alaboudi
Copy link
Contributor

Bug Report

This issue is not a "bug" per se but more of an amendment suggestion to the mapTo operators's type. Didn't know what best to label this as.

Current Behavior
mapTo's current type signature is mapTo<T, R>(value: R): OperatorFunction<T, R> but there is absolutely no need for the T generic for this operator. The whole point of the operator is to start a completely new stream of values with no dependency on the source stream value.

Possible Solution
I think mapTo<R>(value: R): OperatorFunction<any, R> may better represent the intention of using the mapTo operator.

@cartant cartant added the TS Issues and PRs related purely to TypeScript issues label Oct 17, 2019
@kwonoj
Copy link
Member

kwonoj commented Oct 17, 2019

Sounds ok, presume this won't be breaking?

@alaboudi
Copy link
Contributor Author

@kwonoj If we want to not make this a breaking change, then I think we can overload the type definition of this function to the current two generic type along with the new suggested one generic type. Otherwise, this is a breaking change. Since I see @cartant referencing this suggestion in the v7 rollout, is there a point of making this a non breaking introduction when the current implementation doesn't capture the intent of the operator?

@cartant
Copy link
Collaborator

cartant commented Oct 19, 2019

I think we can overload the type definition of this function to the current two generic type along with the new suggested one generic type...

Yeah. ATM, this is a breaking change: it will break anyone who explicitly specifies the T type argument. I agree with the change, as T serves no purpose. We could do the same thing as with of and provide the current signature - that includes the T - as a deprecated signature.

@alaboudi
Copy link
Contributor Author

I like @cartant 's suggestion to add the new signature and mark the current one as a deprecated signature. It won't be a breaking change but will push people towards the correct direction till its phased. If there are no objections, I'll put up a PR soon to address this issue.

@alaboudi
Copy link
Contributor Author

alaboudi commented Oct 24, 2019

@cartant I have put up a PR 3 days ago. Do tell if any changes are required

kwonoj pushed a commit to kwonoj/rxjs that referenced this issue Feb 5, 2020
Reduce the number of generics used in the 'mapTo' function to 1 generic parameter. Mark the 2 generic variant as a deprecated variant

Fixes ReactiveX#5090
martinsik pushed a commit to martinsik/rxjs that referenced this issue Feb 15, 2020
Reduce the number of generics used in the 'mapTo' function to 1 generic parameter. Mark the 2 generic variant as a deprecated variant

Fixes ReactiveX#5090
@lock lock bot locked as resolved and limited conversation to collaborators Feb 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
TS Issues and PRs related purely to TypeScript issues
Projects
None yet
Development

No branches or pull requests

3 participants