Removing resultSelectors (WIP) by benlesh · Pull Request #3304 · ReactiveX/rxjs

benlesh · 2018-02-07T23:24:57Z

This is an experimental PR that is aiming to remove all superfluous resultSelector arguments from operators and observable creators.

To start with I've done mergeMap, mergeMapTo, switchMap, switchMapTo, concatMap, concatMapTo and forkJoin.

See commit messages for more detail.

This is aimed at smaller size, simplifying the API, simplifying the code base, and simplifying TypeScript typings.

Just doing it makes it clear that it's a solid decision, as the code cleans up substantially everywhere it's done.

Relates to #2929

rxjs-bot · 2018-02-07T23:30:05Z

	Warnings
⚠️	❗ Big PR (1)

	Messages
📖

(1) : Pull Request size seems relatively large. If Pull Request contains multiple changes, split each into separate PR will helps faster, easier review.

CJS: 1323.4KB, global: 704.6KB (gzipped: 115.4KB), min: 136.2KB (gzipped: 30.0KB)

Generated by 🚫 dangerJS

coveralls · 2018-02-07T23:34:24Z

Coverage decreased (-0.04%) to 97.522% when pulling 08e8df5 on benlesh:remove-resultSelectors-1 into 52cdfe8 on ReactiveX:master.

kwonoj · 2018-02-08T00:30:05Z

@benlesh before further refactoring in code, please consider #3050 to land prior. it has been resolved conflict several time and waiting review - while functionally it doesn't change anything, it's blocking for TS user would like to use strictFn option which need to be resolved eventually.

benlesh · 2018-02-08T04:33:49Z

@kwonoj that's done.

david-driscoll · 2018-02-20T21:39:27Z

So I wonder if there is a good way to deal with the result selector scenario other than just saying "tough luck".

Going from something like the below just feels worse from a consumers perspective. I totally get it from the authors perspective.

let a4: Rx.Observable<{ o: number; i: string; }> = o.mergeMap(x => [x.toString()], (o, i) => ({ o, i }), 3);

to

let a6: Rx.Observable<{ o: number; i: string; }> = o.map(o => Observable.from([o.toString()]).map(i => ({ o, i }))).mergeAll(3);

I wonder if there is somewhere we can meet in the middle... I'll see if I can figure something out.

kwonoj · 2018-02-20T21:41:08Z

all blocked by mono-repo roadmap, but what we discussed in general is probably considerable to have separate operator package for legacy behaviors for opt-in. It's aligned effort of making core slim, and opt in for other intended behaviors.

benlesh · 2018-02-21T00:32:28Z

@david-driscoll the change would be more like:

o.mergeMap((o => Observable.from(a.toString()).map(i => ({o, i })), 3);

There's no need to use mergeAll

luillyfe · 2018-02-27T16:36:04Z

why are they superfluous ? Actually I can find ResultSelector pretty useful, because I can get access to the index of the source observable and inner observable. Also I can make a custom merge of the observables since I can get access to each item produce in each observable. Is there any place where I can get more info about this topic ?

david-driscoll · 2018-02-28T15:36:16Z

@luillyfe The reason they're seen as superfluous is due to the fact they complicate the inner workings of each operator they're running inside, where it causes problems unit testing all the parts of the operator.

the result selector can easily substituted for something like...

.mergeMap((outer, outerIndex) => Observable.from(outer).map((inner, innerIndex) => ...));

However I agree that is pretty verbose and over the top, so I have a different PR where I'm proposing a helper method to help with the transition (and ensure consistency) #3332

// <type imports here>
import { map } from '../operators/map';
import { from } from '../observable/from';

export function over<O, I, R>(
  selector: (outer: O, index: number) => ObservableInput<I>,
  project: (outerValue: O, innerValue: I, outerIndex: number, innerIndex: number) => R): (outer: O, index: number) => Observable<R> {
  return (outer, outerIndex) => from(selector(outer, outerIndex))
    .pipe(map((inner, innerIndex) => project(outer, inner, outerIndex, innerIndex)));
}

the usage is pretty simple...

.mergeMap(over((outer, inner, outerIndex, innerIndex) => { ... }));

The method is fairly simple but I'd hate to force everyone that wants to use it to define it, so I'll be bringing this up in the next meeting.

benlesh · 2018-02-28T22:39:39Z

@luillyfe they're superfluous because the following is equivalent:

source$.pipe(
  mergeMap((a, i) => of(a + i), (a, b, i, ii) => a + b + i + ii),
)

is the same as:

source$.pipe(
  mergeMap((a, i) => of(a + i).pipe(map((b, ii) => a + b + i + ii)))
)

Only the latter is actually better in many cases, as it allows for catching errors before they propagate out of the mergeMap operation.

The added benefit here is size, as almost always people will be using the map operator, it's generally no extra cost, where we're removing several lines of code from the mergeMap operator.

luillyfe · 2018-03-01T19:43:05Z

Awesome, thank you guys @benlesh @david-driscoll for the clarification.

BREAKING CHANGE: mergeMap, concatMap and concatMapTo no longer support a result selector, if you need to use a result selector, use the following pattern: `source.mergeMap(x => of(x + x).pipe(map(y => y + x))` (the pattern would be the same for `concatMap`).

- Implements mergeMapTo in terms of mergeMap - Removes resultSelector argument BREAKING CHANGE: `mergeMapTo` no longer accepts a resultSelector, to get this functionality, you'll want to use `mergeMap` and `map` together: `source.pipe(mergeMap(() => inner).pipe(map(y => x + y)))`

- removes resultSelector from forkJoin - updates tests

- removes resultSelector argument from `switchMap` and `switchMapTo` - updates tests BREAKING CHANGE: `switchMap` and `switchMapTo` no longer take `resultSelector` arguments, to get the same functionality use `switchMap` and `map` in combination: `source.pipe(switchMap(x => of(x + x).pipe(y => x + y)))`.

- Removes resultSelector argument - updates tests BREAKING CHANGE: `resultSelector` no longer supported, to get this functionality use: `source.pipe(exhaustMap(x => of(x + x).pipe(map(y => x + y))))`

- removes resultSelector argument - updates tests BREAKING CHANGE no longer supports `resultSelector` argument. The same functionality can be achieved by simply mapping either before or after `first` depending on your use case.

- Removes resultSelector argument - Updates tests BREAKING CHANGE: no longer accepts `resultSelector` argument. To get this same functionality, use `map`.

jayphelps · 2018-03-02T01:56:13Z

spec/operators/concatMap-spec.ts

    expectSubscriptions(e1.subscriptions).toBe(e1subs);
  });

+<<<<<<< HEAD


@benlesh whoops

jayphelps · 2018-03-02T01:56:22Z

spec/operators/concatMap-spec.ts

  });

+=======
+>>>>>>> feat(mergeMap|concatMap|concatMapTo): simplified the signatures


@benlesh

- Also fixes issues from a bad rebase that I (@benlesh) did because I'm a dope.

jayphelps · 2018-03-02T02:03:48Z

spec/operators/mergeMap-spec.ts

-
-    function project() { return inner; }
-    function resultSelector(oV: string, iV: string, oI: number, iI: number) { return iV; }
-    const result = e1.mergeMap(project, resultSelector, 1);


It looks like these tests shouldn't have been removed, instead just removing the usage of resultSelector

jayphelps · 2018-03-02T02:03:57Z

spec/operators/mergeMap-spec.ts

-
-    function project() { return inner; }
-    function resultSelector(oV: string, iV: string, oI: number, iI: number) { return iV; }
-    const result = e1.mergeMap(project, resultSelector, 2);


jayphelps · 2018-03-02T02:04:05Z

spec/operators/mergeMap-spec.ts

-
-    function project(x: string) { return inners[x]; }
-    function resultSelector(oV: string, iV: string, oI: number, iI: number) { return iV; }
-    const result = e1.mergeMap(project, resultSelector, 1);


jayphelps · 2018-03-02T02:04:11Z

spec/operators/mergeMap-spec.ts

-
-    function project(x: string) { return inners[x]; }
-    function resultSelector(oV: string, iV: string, oI: number, iI: number) { return iV; }
-    const result = e1.mergeMap(project, resultSelector, 2);


jayphelps · 2018-03-02T02:04:55Z

spec/operators/mergeMapTo-spec.ts

-                     '                                         ^                   !'];
-    const expected =   '-----i---j---k---l-------i---j---k---l-------i---j---k---l---|';
-
-    function resultSelector(oV: string, iV: string, oI: number, iI: number) { return iV; }


exists below "without resultSelector". I refactored to use the pipeable operator anyhow.

jayphelps · 2018-03-02T02:05:08Z

spec/operators/mergeMapTo-spec.ts

-                     '                     ^                   !'];
-    const expected =   '-----i---j---(ki)(lj)k---(li)j---k---l---|';
-
-    function resultSelector(oV: string, iV: string, oI: number, iI: number) { return iV; }


Oh... yeah, there's the same test below it "without resultSelector"

- also readds important tests removed in a previous commit in this PR

benlesh · 2018-03-02T23:09:52Z

Thanks for the review @jayphelps!

Note to future viewers of this PR: The red "X" was only because code coverage dropped 0.03% (or something like that)... So it's acceptable and merged anyhow.

lock · 2018-06-06T03:35:10Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

benlesh requested a review from kwonoj February 7, 2018 23:25

benlesh force-pushed the remove-resultSelectors-1 branch from 91b9dee to 99f21d4 Compare February 8, 2018 03:45

david-driscoll mentioned this pull request Feb 21, 2018

[wip] feat(helper): new helper method over to replace the resultSelector used by methods like mergeMap #3332

Closed

jsonberry mentioned this pull request Feb 27, 2018

docs(operators): add documentation for concatMap ReactiveX/rxjs-docs#272

Open

benlesh force-pushed the remove-resultSelectors-1 branch from 7a33063 to de57152 Compare February 28, 2018 22:53

benlesh added 7 commits March 1, 2018 16:25

feat(forkJoin): simplify interface

4d2338b

- removes resultSelector from forkJoin - updates tests

feat(exhaustMap): simplify interface

42589d0

- Removes resultSelector argument - updates tests BREAKING CHANGE: `resultSelector` no longer supported, to get this functionality use: `source.pipe(exhaustMap(x => of(x + x).pipe(map(y => x + y))))`

feat(first): simplify interface

a011338

- removes resultSelector argument - updates tests BREAKING CHANGE no longer supports `resultSelector` argument. The same functionality can be achieved by simply mapping either before or after `first` depending on your use case.

feat(last): simplify interface

3240419

- Removes resultSelector argument - Updates tests BREAKING CHANGE: no longer accepts `resultSelector` argument. To get this same functionality, use `map`.

benlesh force-pushed the remove-resultSelectors-1 branch from de57152 to 193ff36 Compare March 2, 2018 00:25

jayphelps suggested changes Mar 2, 2018

View reviewed changes

refactor(types): fix typing issues

7ff121c

- Also fixes issues from a bad rebase that I (@benlesh) did because I'm a dope.

benlesh force-pushed the remove-resultSelectors-1 branch from 193ff36 to 7ff121c Compare March 2, 2018 02:05

jayphelps suggested changes Mar 2, 2018

View reviewed changes

benlesh added 2 commits March 1, 2018 20:17

refactor(mergeMap-tests): update tests to use pipe

0f015c4

- also readds important tests removed in a previous commit in this PR

refactor(mergeMapTo-spec): use pipeable operator

08e8df5

jayphelps approved these changes Mar 2, 2018

View reviewed changes

benlesh merged commit 9319d9b into ReactiveX:master Mar 2, 2018

jasonaden mentioned this pull request Mar 14, 2018

map() does not work with forkJoin #3432

Closed

lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018

Conversation

benlesh commented Feb 7, 2018

Uh oh!

rxjs-bot commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kwonoj commented Feb 8, 2018

Uh oh!

benlesh commented Feb 8, 2018

Uh oh!

david-driscoll commented Feb 20, 2018

Uh oh!

kwonoj commented Feb 20, 2018

Uh oh!

benlesh commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luillyfe commented Feb 27, 2018

Uh oh!

david-driscoll commented Feb 28, 2018

Uh oh!

benlesh commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luillyfe commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benlesh commented Mar 2, 2018

Uh oh!

lock bot commented Jun 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rxjs-bot commented Feb 7, 2018 •

edited

Loading

coveralls commented Feb 7, 2018 •

edited

Loading

benlesh commented Feb 21, 2018 •

edited

Loading

benlesh commented Feb 28, 2018 •

edited

Loading