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

Single operator fixes #5325

Merged
merged 4 commits into from
May 14, 2020
Merged

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Feb 26, 2020

Per our discussion of #5320, this is a set of fixes to the behavior of single, which was wildly incorrect. Modernizes and updates tests. Rewrites the operator in the newer functional style.

BREAKING CHANGES:

  • single() will now throw if more than one value comes from the source. Previously it would not emit.
  • single(predicate), in cases where nothing matches the predicate, will now throw a NotFoundError if nothing matched, and an EmptyError if the source simply did not emit any values.
  • single(predicate) on an empty source will now emit an EmptyError. Previously it would erroneously emit undefined.

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.

A few nits about some marbles and the TSDoc.

src/internal/operators/single.ts Outdated Show resolved Hide resolved
src/internal/operators/single.ts Show resolved Hide resolved
import { single, mergeMap, tap } from 'rxjs/operators';
import { of, EmptyError } from 'rxjs';

declare function asDiagram(arg: string): Function;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK what the consequences of removing this asDiagram business are? Will it break website images when the docs are re-created?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I remember, we stop generating them like this, and we're just using old, static images. @jwo719, can you confirm this? I've been removing these asDiagram calls when I see them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol I never noticed. So it's kinda true, that we are using a static old version of the generated marbleDiagrams, but there's a pr and several issues to improve this mechanism and IIRC it's still using this asDiagram function, so it would be good to keep it for now. I can go through the history to find the deleted images.

This explains now why there is always a diff when I try to generate the new images :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@niklas-wortmann ... can you please set up a separate issue? I've been removing these for the last year at least, because at one point we stopped using them. It's always the first test in the file that had it before. It would be nice if this was something that was imported and used, rather than a global variable, too.

spec/operators/single-spec.ts Outdated Show resolved Hide resolved
- Will now throw new NotFoundError and SequenceError types in appropriate scenarios
- Updates documentation

BREAKING CHANGE: Single will now throw for scenarios where values coming in are either not present, or do not match the provided predicate. Error types have thrown have also been updated, please check documentation for changes.
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

@benlesh benlesh merged commit 27931bc into ReactiveX:master May 14, 2020
ajitsinghkaler added a commit to ajitsinghkaler/rxjs that referenced this pull request May 14, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants