test: enable remaining synchronous firehose tests #5749
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
17 tasks
cartant
commented
Sep 24, 2020
src/internal/operators/throttle.ts
Outdated
| // If a synchronous duration selector is specified - weird, but legal - | ||
| // an already-closed subscription will be assigned to throttled, so the | ||
| // subscription's closed property needs to be checked, too. | ||
| (!throttled || throttled.closed) && (leading ? send() : throttle(value)); |
There was a problem hiding this comment.
This is the only significant change/fix in the PR.
cartant
commented
Sep 24, 2020
Comment on lines
+705
to
+720
| // AFAICT, it's not possible for multicast observables to support ASAP | ||
| // unsubscription from synchronous firehose sources. The problem is that the | ||
| // chaining of the closed 'signal' is broken by the subject. For example, | ||
| // here: | ||
| // | ||
| // https://github.com/ReactiveX/rxjs/blob/2d5e4d5bd7b684a912485e1c1583ba3d41c8308e/src/internal/operators/multicast.ts#L53 | ||
| // | ||
| // The subject is passed to subscribe. However, in the subscribe | ||
| // implementation a SafeSubcriber is created with the subject as the | ||
| // observer: | ||
| // | ||
| // https://github.com/ReactiveX/rxjs/blob/2d5e4d5bd7b684a912485e1c1583ba3d41c8308e/src/internal/Observable.ts#L210 | ||
| // | ||
| // That breaks the chaining of closed - i.e. even if the unsubscribe is | ||
| // called on the subject, closing it, the SafeSubscriber's closed property | ||
| // won't reflect that. |
There was a problem hiding this comment.
FYI, multicast-based operators are not going to pass the synchronous firehose test with the current implementation of the closed 'signal'.
There was a problem hiding this comment.
I suppose this could be fixed by bringing back the SubjectSubscriber.
Just to be clear, though, I think we should merge this PR as-is and address this multicast-firehose business in another PR (if at all).
cartant
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This PR enables the remaining synchronous firehose tests and fixes operators to ensure they pass. Several operators have tests that now pass due to changes made in other refactoring PRs.
Related issue (if exists): #5658