feat: stopped notification handler #5750
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
cartant
commented
Sep 24, 2020
cartant
commented
Sep 24, 2020
benlesh
reviewed
Sep 24, 2020
benlesh
reviewed
Sep 24, 2020
benlesh
reviewed
Sep 24, 2020
cartant
force-pushed
the
stopped-notification
branch
from
f144601
to
1cf1cd3
Compare
cartant
changed the title
benlesh
reviewed
Sep 29, 2020
benlesh
requested changes
Sep 29, 2020
|
Also needs a rebase it seems. |
cartant
force-pushed
the
stopped-notification
branch
from
3b3d330
to
42a42ce
Compare
benlesh
reviewed
Oct 2, 2020
benlesh
reviewed
Oct 2, 2020
benlesh
reviewed
Oct 2, 2020
benlesh
requested changes
Oct 2, 2020
cartant
force-pushed
the
stopped-notification
branch
from
84d8615
to
2814491
Compare
|
Addressed comments. |
benlesh
approved these changes
Oct 30, 2020
cartant
force-pushed
the
stopped-notification
branch
from
94ef4d4
to
72170ff
Compare
cartant
force-pushed
the
stopped-notification
branch
from
72170ff
to
fe27cd4
Compare
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:
@benlesh
This PR is a WIP representation of what I would like to see implemented as a replacement for the
canReportErrorshenanigans - which I would delete as part of this being merged.The fundamental problem is that errors reported to stopped subscribers are swallowed and no mechanism is offered to RxJS developers to let them alter this behaviour. ATM, there is one place - in the
try/catchthat surrounds thesubscribewithinObservable- at which the chain of subscribers is traversed to determine whether or not any are stopped/closed and whether the error can be reported. This is pretty crappy - I feel comfortable saying this, because I wrote the code. 😅I would prefer to ditch the traversal entirely and, instead, offer a configuration point that can be used to register a handler for notifications sent to stopped observers. This would be more general than the traversal that currently happens - I can see it being useful in the DevTools - and would certainly be simpler. IMO, this shouldn't really impact anyone. ATM, in v6, errors that cannot be reported are written to the console; it's only in v7 that an error is actually thrown. Also, to my knowledge, this error swallowing has only ever affected me - when investigating two bugs in the RxJS core.
What are your thoughts on this? AFAICT, it the wiring up of the stopped-notification handler only needs to happen in
Subscriber#next/error/complete. If you want me to go ahead with this, I can add some tests and can removecanReportError, etc.IMO, this doesn't have to be perfect; it just has to be better than the traversal - and that shouldn't be too hard. 😅
Related issue (if exists): None