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

fix(subscribe): ignore syncError when deprecated #3749

Merged
merged 2 commits into from
May 31, 2018

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented May 26, 2018

Description:

This PR adds a failing test and fixes the problem with the implementation of subscribe that sees the catchError operator bypassed in some circumstances.

This will fix the behaviour when useDeprecatedSynchronousErrorHandling is false - i.e. the default version 6 behaviour.

Elsewhere, the syncErrorThrowable flag has no meaning unless useDeprecatedSynchronousErrorHandling is true.

So, when useDeprecatedSynchronousErrorHandling is false, it makes no sense to consider syncErrorThrowable when deciding between a call to _subscribe or _trySubscribe.

Whether or not - as discussed in the related issue - a getter should be added to InnerSubscriber to attempt to fix the deprecated behaviour is another (more complicated) matter. And one that need not be relevant to this PR. As noted in this comment, even with the getter added, the deprecated behaviour is still broken.

Related issue (if exists): #3740

Unless using the deprecated syncError handling, the syncErrorThrowable
flag should be ignored.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 96.654% when pulling 44e1a27 on cartant:issue-3470 into d7bfc9d on ReactiveX:master.

@benlesh benlesh merged commit f94560c into ReactiveX:master May 31, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jun 30, 2018
@cartant cartant deleted the issue-3470 branch September 24, 2020 07:07
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