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

feat(switchScan): add switchScan() operator #4442

Merged
merged 12 commits into from
Oct 5, 2020

Conversation

martinsik
Copy link
Contributor

@martinsik martinsik commented Dec 30, 2018

  • Add the operator to Rx
  • It must have a -spec.ts tests file covering the canonical corner cases, with marble diagram tests
  • If possible, write a asDiagram test case too, for PNG diagram generation purposes
  • The spec file should have a type definition test at the end of the spec to verify type definition for various use cases
  • The operator must be documented in JSDoc style in the implementation file, including also the PNG marble diagram image
  • The operator should be listed in doc/operators.md in a category of operators
  • The operator should also be documented. See Documentation Guidelines.
  • It should also be inserted in the operator decision tree file doc/decision-tree-widget/tree.yml
    You may need to update MIGRATION.md if the operator differs from the corresponding one in RxJS v4

Description:

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 and tap because I first started implementing it myself but then realised I'm doing almost the same thing as switchMap 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 for scan and mergeScan.

I made it to work similarly to scan because that's more close to Array.reduce() even though mergeScan 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 passes index 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

@coveralls
Copy link

coveralls commented Dec 30, 2018

Pull Request Test Coverage Report for Build 8447

  • 14 of 14 (100.0%) changed or added relevant lines in 3 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.03%) to 96.816%

Files with Coverage Reduction New Missed Lines %
src/internal/Observer.ts 1 75.0%
src/internal/Subscriber.ts 1 81.48%
Totals Coverage Status
Change from base Build 8442: -0.03%
Covered Lines: 5808
Relevant Lines: 5999

💛 - Coveralls

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Jan 9, 2019
@benlesh
Copy link
Member

benlesh commented Jan 9, 2019

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!

cartant
cartant previously requested changes Feb 1, 2019
Copy link
Collaborator

@cartant cartant left a 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.

src/internal/operators/switchScan.ts Outdated Show resolved Hide resolved
src/internal/operators/switchScan.ts Outdated Show resolved Hide resolved
@cartant cartant self-requested a review March 19, 2019 02:19
@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Apr 10, 2019
Copy link
Member

@benlesh benlesh left a 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!

spec/operators/switchScan-spec.ts Outdated Show resolved Hide resolved
spec/operators/switchScan-spec.ts Outdated Show resolved Hide resolved
spec-dtslint/operators/switchScan-spec.ts Outdated Show resolved Hide resolved
spec-dtslint/operators/switchScan-spec.ts Show resolved Hide resolved
@martinsik
Copy link
Contributor Author

@benlesh @cartant Hey, I updated this PR with everything you mentioned above.

@cartant cartant self-requested a review May 4, 2019 12:05
@BioPhoton
Copy link
Contributor

I stumble over this open PR and see that all requested changes are implemented.
The failed build on node v9 is related to some non-code related things which I guess could be ignored.
@benlesh may I kindly ask you for a status update to this PR= :)

@tonivj5
Copy link
Contributor

tonivj5 commented Apr 2, 2020

Any update on this PR? 👍

@martinsik
Copy link
Contributor Author

Rebased to master and fixed some types in spec.

@cartant
Copy link
Collaborator

cartant commented Apr 8, 2020

Thanks. I'll re-review it.

@benlesh
Copy link
Member

benlesh commented Sep 20, 2020

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

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Sep 20, 2020
@benlesh benlesh dismissed stale reviews from cartant and themself October 2, 2020 20:52

Taken over.

@benlesh
Copy link
Member

benlesh commented Oct 2, 2020

Resurrected! 🧟

Copy link
Collaborator

@cartant cartant left a 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.

spec/operators/switchScan-spec.ts Outdated Show resolved Hide resolved
expectSubscriptions(e1.subscriptions).toBe(e1subs);
});

it('should handle a synchronous switch to the second inner observable', () => {
Copy link
Collaborator

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.

* <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>
*
* ![](switchScan.png)
Copy link
Collaborator

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?

Copy link
Member

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.

src/internal/operators/switchScan.ts Outdated Show resolved Hide resolved
src/internal/operators/switchScan.ts Show resolved Hide resolved
@niklas-wortmann niklas-wortmann mentioned this pull request Oct 5, 2020
8 tasks
@benlesh benlesh merged commit 73fa910 into ReactiveX:master Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AGENDA ITEM Flagged for discussion at core team meetings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: add a switchScan operator
6 participants