-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Proposal] Allow blocs to finish processing pending events on close #639
Comments
IMO, this makes total sense... but I wonder if there should be a |
There is a valid request for this in #611 |
This actually sounds good. It would help tests and it probably would not change current behavior with Let me ask a different question: where don't we want such a behavior? |
@tenhobi yup it would have no impact on the functionality of any existing flutter_bloc widgets. I personally can’t think of a scenario where we wouldn’t want this behavior either. |
There should be a configurable flag to enable/disable this. There are situations when you don't want to finish processing an event. e.g.: a user wants to navigate to a different route while the current bloc is processing an event which will result in some new UI state that the user probably doesn't care anymore since he initiated a navigation to a completely new screen. There's no point in waiting on that event and the state to be yielded before closing that bloc. |
@RollyPeres in those cases there wouldn’t be any waiting on the event. The only difference is the state change would happen as opposed to right now the logic still executes but when the state change is about to happen it gets ignored. When disposing blocs, BlocProvider would not need to wait for close to complete and if you want to wait you can handle disposing manually and await close yourself. Let me know what you think and thanks for the feedback! |
Or even better, have all events be fully processed by default and have a |
What do you mean by have a List? Can you provide some sample code? |
Maybe the bloc could have a
This could probably be improved when dart team will release something like |
@RollyPeres thanks for the clarification. I'm trying to better understand under which circumstances would you not want pending events to be processed to completion? In the current state, bloc is not very "predictable" in this regard because depending on timing you might get different outcomes when |
I agree with @jorgecoca for having closeWithoutWaiting which can be helpful when calling fire-and-forget API or similar |
I agree that it should process pending events by default and that if you want it to stop immediately, which I don't see a use case for, you can do, It should not have a separate method that does the same thing for simplicity reasons IMO. |
@binoypatel do you have a use case you could share where you would want the fire and forget functionality? |
@felangel Say the user is on screen A, initiates an event and then clicks away from it by going to screen B. If the setup is that screen A gets disposed together with the bloc A, and say that event is just some data fetch with no side effects, then what is the point of waiting for that event to finish ? It's just gonna yield a state that the user doesn't care about anymore. I'm sure like 90% of cases revolve around fully processing all the events for predictability as you mentioned, but there's gotta be a couple of niche cases where having the option to skip over some events might be great. |
@RollyPeres I'm always in support of those just in case cases |
@RollyPeres in those cases the only thing that would change is the bloc would have one additional transition occur. It wouldn’t block the UI or navigation and currently the bloc still finishes processing events but when it comes time to emit the new states they are all ignored so it wouldn’t introduce any additional overhead. Let me know if that helps clarify things. |
@ThinkDigitalSoftware finally, a believer 😂 |
@RollyPeres in that case those events will not be added because no matter what once close is called no new events can be added. I completely agree with you but I'd like to have an understanding of concrete edge cases instead of speculating. So far I can't think of any cases where the current implementation would be desirable over the proposed implementation. |
@RollyPeres @felangel It would make sense to me if it behaved the same way stores do when they close. Don't allow in new customers, but allow the existing customers to complete their checkout. I would say let the bloc finish it's own internal processing, even if that includes adding another event to the stream, but I don't see how that would be able to be controlled |
@ThinkDigitalSoftware I don't think we should allow new events to be added. Wouldn't that be like existing customers letting in their friends after the store has closed? |
It would be like parents having their children checkout separately |
it's like a Future than has a then clause. Except in bloc, you either process the "then" inside the same run, or dispatch an event if you already have code set up to respond to that event. |
@ThinkDigitalSoftware I think those are different because the bloc shouldn't care where the event came from in my opinion. We shouldn't be differentiating between internal events and external events. |
I disagree because with the children analogy, the children would already be in the store (events were already added). In the case you're describing we'd be adding new events. |
I agree that it wouldn't be able to differentiate easily, but it seems like if someone happened to use that pattern it may take some debugging to figure out why their event didn't process. I guess an error thrown if an event was added after it was closed would work, but customized for |
@ThinkDigitalSoftware there is already an error thrown in
|
I
I was thinking more like the kids were attached to their parents and came in at once, but then things changed. With bloc extending stream, this no longer makes sense since an |
Oh, then yes, it's perfect :) Or if you wanted to go a step further, you could mention the event that was added, just in case someone uses this pattern. I've seen many a bug happen because of some forgotten side effect. But that's just extra protection against bad programming behaviors |
Basically, all long running / background task are good fit for fire-and-forget, for example, user set settings for importing active directory users (let say there are more than 10,000 users) and then click on import button on the screen, and then navigate to the home page, import will trigger fire-and-forget api which will take long time to complete and not require user to wait until the process is finished |
@binoypatel sorry if I was confusing but none of the proposed changes would force a user to wait until a process is finished. All bloc processing is happening asynchronously and would not block the UI. Furthermore in your example, if close is called in the middle of an import would you want the import to be partially complete? I would imagine you wouldn't want your app to be in a state where an import was partially executed (this is what I was referring to when I said the current implementation is not predictable). Instead, the proposal would allow for the bloc to finish the pending import asynchronously while the user navigates to the home page. |
Thanks for the details, It is more clear now, I think in this case it will make sense to await before closing but then the caller will be forced to be async'ed which may introduce lot of changes for consumer |
@binoypatel no problem! You would only need to make changes if you're overriding @override
void close() {
_subscription.cancel();
super.close();
} You'd need to change to: @override
Future<void> close() {
_subscription.cancel();
return super.close();
} You wouldn't need to |
Merged in #638 and will be included in v2.0.0 of bloc. |
IMO it doesn't make sense to emit things after the stream is closed. At best it's hacky, at worse it cause undesired effects. That's why try/finally is there IMO: () async* {
print('start');
try {
await Future.delayed(const Duration(seconds: 5));
yield MyValue();
} finally {
print('finished');
}
} The |
@rrousselGit we wouldn't be emitting things after the stream is closed. The proposal was to close the sink, finish processing/emitting any pending events, and then close the stream. This would ensure that if an event is added to the bloc before Edit: I saw your response on twitter and it looks like we're in agreement 😄 |
Is your feature request related to a problem? Please describe.
Currently, if
close
is called on a bloc while an event is being processed, the subsequent state changes (transitions
) will be ignored.For example, given the following bloc:
The current behavior is:
Describe the solution you'd like
It might be better to modify
close
to allow for any pending events to be processed to completion. This would help with testing and would also allow developers to know when a bloc has been closed as the process is async.See #611 for more more context and #638 for a draft.
The text was updated successfully, but these errors were encountered: