-
Notifications
You must be signed in to change notification settings - Fork 114
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 stream cancelling for FetchPolicy.CacheAndNetwork, too #53
Conversation
…ing a await for / yield loop and make the method synchronous. partial fix for gql-dart#51
Thanks for the PR! I remember there was a reason we used async await for the network request, but I'm travelling now and can't dig into it. I'll try to take another look next week. |
…am. change test that verifies behaviour when link throws an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to further investigate the FetchPolicy.CacheAndNetwork code at some point to better understand what's going on, but I think these changes are fine for now. There may be a way to refactor that doesn't require the use of shareValue
at all.
Thanks again for the PR!
ferry/test/error_test.dart
Outdated
} | ||
|
||
abstract class Mockable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to leave the Mockable & MyMock classes in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad. Of course not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This fixes #51 .
This includes the changes in #52, and fixes for
FetchPolicy.cacheAndNetwork
policy, and extends the test for canceling the streams when they are not listened to anymore for all FetchPolicies.I am not really sure why these fixes to
FetchPolicy.cacheAndNetwork
are necessary, I know too little about rxdart for this. But with wrapping the network stream in a new StreamController and manually canceling it, the canceling of the stream is passed on the Link, too.There might be a better with rxdart, which is why I put this fix in a separate PR.