-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(switchScan): add switchScan() operator #4442
Conversation
Pull Request Test Coverage Report for Build 8447
💛 - Coveralls |
Very interesting, @martinsik... I know this is something @trxcllnt wanted to add back in the day. I've marked this for core team discussion. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a complete review. I'm just pointing out some things I've noticed whilst perusing the changes. I'll have a closer look at them later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delayed review. Let's get this in!
I stumble over this open PR and see that all requested changes are implemented. |
Any update on this PR? 👍 |
076c926
to
9ea12f9
Compare
Rebased to master and fixed some types in spec. |
Thanks. I'll re-review it. |
Oh man... this old thing. Sigh. Casualty of burnout mostly. I'll mark it as an agenda item and see if the team still wants to land it, or if it would be better in rxjs-misc. cc @cartant |
091ac29
to
5c788b3
Compare
Resurrected! 🧟 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a bunch of nitpicks.
expectSubscriptions(e1.subscriptions).toBe(e1subs); | ||
}); | ||
|
||
it('should handle a synchronous switch to the second inner observable', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: This looks like one of those tests that purports to test something that's synchronous, but isn't really synchronous-within-subscribe
. Nothing to do here, this is just an observation related to the sync-within-subscribe
marble syntax that we talked about.
src/internal/operators/switchScan.ts
Outdated
* <span class="informal">It's like {@link scan}, but only the most recent | ||
* Observable returned by the accumulator is merged into the outer Observable.</span> | ||
* | ||
*  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: @niklas-wortmann With the addition of a new operator, is there some config that needs to updated so that a marble diagram for it is generated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a TODO around this.
-spec.ts
tests file covering the canonical corner cases, with marble diagram testsasDiagram
test case too, for PNG diagram generation purposesdoc/operators.md
in a category of operatorsdoc/decision-tree-widget/tree.yml
You may need to updateMIGRATION.md
if the operator differs from the corresponding one in RxJS v4Description:
This PR adds
switchScan
operator as described here #2931 and reopened by @benlesh some time ago. I was told on Slack that it might get refused but I'm ok with that (no harsh feelings :)).It's internally just using
switchMap
andtap
because I first started implementing it myself but then realised I'm doing almost the same thing asswitchMap
is doing already so I just used combination of those two operators (I think it's been recommended to rather reuse existing operators than creating them from scratch). Tests are a combination of existing tests forscan
andmergeScan
.I made it to work similarly to
scan
because that's more close toArray.reduce()
even thoughmergeScan
behaves a little differently. I tried to describe it here #4348 (comment) and here #4441 (the way it handles the first emission from source without seed and that it passesindex
to the accumulator function).I also didn't add this operator to
rxjs-compat
because it didn't exist previously.Related issue (if exists):
Closes #2931