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.

Sign up for GitHub

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

Jump to bottom

Deprecating MapTo variants. #6399

Closed
benlesh opened this issue May 12, 2021 · 3 comments
Closed

Deprecating MapTo variants. #6399

benlesh opened this issue May 12, 2021 · 3 comments
Labels
8.x Issues and PRs for version 8.x

Comments

@benlesh
Copy link
Member

benlesh commented May 12, 2021

Based off of a comment here

At one point, there was some performance advantage to having mapTo, concatMapTo, switchMapTo, et al. Because they introduced a way to map to the same value over and over again, without introducing a closure.

Now:

  1. Closures are much faster.
  2. Therefor the whole library is written in such a way that is uses closures.

Considering that mergeMapTo(x) and mergeMap(() => x) do pretty much the same thing, only one is hiding the closure, I thikn we should deprecate the MapTo variants to help clean up the core API.
@benlesh benlesh added 8.x Issues and PRs for version 8.x AGENDA ITEM Flagged for discussion at core team meetings labels May 12, 2021
@backbone87
Copy link
Contributor

backbone87 commented May 12, 2021 •
edited
Loading

some usages experience a decrease in ergonomy:

mapTo(new Foo());
// vs
const foo = new Foo();
map(() => foo);
// vs
map(((foo) => () => foo)(new Foo()));

personally, I have used this behavior on a few occasions. this isn't a big deal, just wanted to make sure this case is considered.

@benlesh
Copy link
Member Author

benlesh commented May 19, 2021

Core Team meeting:

👍 to deprecate
❓ Maybe not remove until version 9 (@kwonoj)

@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label May 19, 2021
@benlesh benlesh mentioned this issue May 19, 2021
Open
40 tasks
@sod
Copy link

sod commented Jul 2, 2021

This is genius. The documentation becomes easier to digest for newcomers and operators are easier to discover. And push developers into writing their first operator themself to harness the full potential of rxjs, as @backbone87 already mentioned, having a mapTo is kinda useful :)

@rafaelss95 rafaelss95 mentioned this issue Sep 30, 2021
Merged
@jakovljevic-mladen jakovljevic-mladen mentioned this issue Dec 7, 2021
Merged
@peaBerberian peaBerberian mentioned this issue Dec 15, 2021
Merged
peaBerberian added a commit to canalplus/rx-player that referenced this issue Dec 15, 2021
@peaBerberian
update dependencies and code
267ab13 
This is the usual dependency update before a release (scheduled
to be the v3.26.2).

After updating `typescript-eslint`, new linter errors made their
apparitions, for cases where an `any` type is used as a function
argument (the rule in question is
[no-unsafe-argument](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unsafe-argument.md)).

That's a rule we want (`any` is ___bad___ after all!) but RxJS,
our main dependency, does not respect it well.
Especially, some operators which transform an observable into
another without needing to look at the items of the source
Observable (e.g. `mapTo`, `mergeMapTo`, `switchMapTo`,
`ignoreElements`) have an `any` in their type definition for the type
of the source Observable's elements (where an `unknown` would actually
suffice and be type-safe).

Replacing on their side this `any` by `unknown` is planned for their
8.x.x release (which is not yet released). In the meantime, we had to
find a solution.

I chose to do two things:

  1. Replace all the "***To" (`mapTo`, `mergeMapTo` and so on)
     operators by their callback-based equivalent (`map`,
     `mergeMap` and so on).
     I did that mostly because RxJS devs [seems to plan
     deprecating the ***To operators anyway in profit of the
     callback-based ones](ReactiveX/rxjs#6399).

  2. For the last remaining operator, `ignoreElements()`, I just
  disabled the rule with a long comment attached on top of each case.
peaBerberian added a commit to canalplus/rx-player that referenced this issue Dec 15, 2021
@peaBerberian
update dependencies and code
ca0f0bc 
This is the usual dependency update before a release (scheduled
to be the v3.26.2).

After updating `typescript-eslint`, new linter errors made their
apparitions, for cases where an `any` type is used as a function
argument (the rule in question is
[no-unsafe-argument](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unsafe-argument.md)).

That's a rule we want (`any` is ___bad___ after all!) but RxJS,
our main dependency, does not respect it well.
Especially, some operators which transform an observable into
another without needing to look at the items of the source
Observable (e.g. `mapTo`, `mergeMapTo`, `switchMapTo`,
`ignoreElements`) have an `any` in their type definition for the type
of the source Observable's elements (where an `unknown` would actually
suffice and be type-safe).

Replacing on their side this `any` by `unknown` is planned for their
8.x.x release (which is not yet released). In the meantime, we had to
find a solution.

I chose to do two things:

  1. Replace all the "***To" (`mapTo`, `mergeMapTo` and so on)
     operators by their callback-based equivalent (`map`,
     `mergeMap` and so on).
     I did that mostly because RxJS devs [seems to plan
     deprecating the ***To operators anyway in profit of the
     callback-based ones](ReactiveX/rxjs#6399).

  2. For the last remaining operator, `ignoreElements()`, I just
  disabled the rule with a long comment attached on top of each case.
peaBerberian added a commit to canalplus/rx-player that referenced this issue Dec 15, 2021
@peaBerberian
update dependencies and code
07b88fa 
This is the usual dependency update before a release (scheduled
to be the v3.26.2).

After updating `typescript-eslint`, new linter errors made their
apparitions, for cases where an `any` type is used as a function
argument (the rule in question is
[no-unsafe-argument](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unsafe-argument.md)).

That's a rule we want (`any` is ___bad___ after all!) but RxJS,
our main dependency, does not respect it well.
Especially, some operators which transform an observable into
another without needing to look at the items of the source
Observable (e.g. `mapTo`, `mergeMapTo`, `switchMapTo`,
`ignoreElements`) have an `any` in their type definition for the type
of the source Observable's elements (where an `unknown` would actually
suffice and be type-safe).

Replacing on their side this `any` by `unknown` is planned for their
8.x.x release (which is not yet released). In the meantime, we had to
find a solution.

I chose to do two things:

  1. Replace all the "***To" (`mapTo`, `mergeMapTo` and so on)
     operators by their callback-based equivalent (`map`,
     `mergeMap` and so on).
     I did that mostly because RxJS devs [seems to plan
     deprecating the ***To operators anyway in profit of the
     callback-based ones](ReactiveX/rxjs#6399).

  2. For the last remaining operator, `ignoreElements()`, I just
  disabled the rule with a long comment attached on top of each case.
peaBerberian added a commit to canalplus/rx-player that referenced this issue Dec 15, 2021
@peaBerberian
update dependencies and code
8a49c7c 
This is the usual dependency update before a release (scheduled
to be the v3.26.2).

After updating `typescript-eslint`, new linter errors made their
apparitions, for cases where an `any` type is used as a function
argument (the rule in question is
[no-unsafe-argument](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unsafe-argument.md)).

That's a rule we want (`any` is ___bad___ after all!) but RxJS,
our main dependency, does not respect it well.
Especially, some operators which transform an observable into
another without needing to look at the items of the source
Observable (e.g. `mapTo`, `mergeMapTo`, `switchMapTo`,
`ignoreElements`) have an `any` in their type definition for the type
of the source Observable's elements (where an `unknown` would actually
suffice and be type-safe).

Replacing on their side this `any` by `unknown` is planned for their
8.x.x release (which is not yet released). In the meantime, we had to
find a solution.

I chose to do two things:

  1. Replace all the "***To" (`mapTo`, `mergeMapTo` and so on)
     operators by their callback-based equivalent (`map`,
     `mergeMap` and so on).
     I did that mostly because RxJS devs [seems to plan
     deprecating the ***To operators anyway in profit of the
     callback-based ones](ReactiveX/rxjs#6399).

  2. For the last remaining operator, `ignoreElements()`, I just
  disabled the rule with a long comment attached on top of each case.
benlesh added a commit to benlesh/rxjs that referenced this issue Mar 4, 2022
@benlesh
chore: deprecate MapTo variants
6a6798e 
Deprecating MapTo variants, as they were only wrappers around the Map variants, and added unnecessary API surface area.

related ReactiveX#6367
resolves ReactiveX#6399
@benlesh benlesh closed this as completed in 45a22e2 Mar 8, 2022
@benlesh benlesh mentioned this issue Mar 10, 2022
Closed
bglamadrid added a commit to trebol-ecommerce/ngx-trebol-frontend that referenced this issue Apr 20, 2022
@bglamadrid
chore: revert switchMapTos back to switchMap
828d81a 
this change looks forward to rxjs v8
for more info see ReactiveX/rxjs#6399
bglamadrid added a commit to trebol-ecommerce/ngx-trebol-frontend that referenced this issue Apr 20, 2022
@bglamadrid
chore: do not use mapTos variants
19ab816 
this change looks forward to rxjs v8
for more info see ReactiveX/rxjs#6399
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
8.x Issues and PRs for version 8.x
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@sod @backbone87 @benlesh