Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Mutation effects should be decoupled from mount #114

Closed
micimize opened this issue Oct 8, 2018 · 3 comments
Closed

Mutation effects should be decoupled from mount #114

micimize opened this issue Oct 8, 2018 · 3 comments
Assignees
Labels
next Feature or report is related to next
Milestone

Comments

@micimize
Copy link
Collaborator

micimize commented Oct 8, 2018

Describe the bug
Unmounting a mutation cancels the onCompleted subscription, and thus results in logic unexpectedly not executing.

To Reproduce
Render a "loading" indicator in place of a Mutation(onCompleted: () => print('completed'), ...) wrapped button

Expected behavior
I think it would be more intuitive if we handled inflight mutations in the query manager itself. Apollo still uses promises for mutations - maybe we can revive some of the old Future logic for the mutation flow?

@HofmannZ
Copy link
Member

@micimize Yeah, in flight mutations should be handled in the query manager. I think we should revisit the stream cleanup, to check if there are in-flight queries (or mutations) before closing the stream.

What do you think?

@HofmannZ HofmannZ added the next Feature or report is related to next label Oct 28, 2018
@HofmannZ HofmannZ added this to the 1.0.0 milestone Oct 28, 2018
@HofmannZ HofmannZ changed the title [next] Mutation effects should be decoupled from mount Mutation effects should be decoupled from mount Oct 28, 2018
@micimize
Copy link
Collaborator Author

@HofmannZ ended up implementing essentially what you're talking about in #130 using a lifecycle enum to track mutations, and waiting to clean them up after side effects.

One major change is that I'm deferring the creation of the observable until runMutation, and forcibly replacing it if the same mutation is run multiple times, replacing callbacks. These changes work for now, but I still feel like the way it hangs together is a bit non-optimal.

With queries, I think we can assume the results won't be used if the component is unmounting, so I think they're fine.

sidebar - I wasn't sure about sticking with Streams for mutations over Futures, but the more I thought about it, there's no reason users shouldn't eventually be able to use the seemingly query-specific directives like @defer in mutations

@micimize
Copy link
Collaborator Author

was closed by #130

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
next Feature or report is related to next
Projects
None yet
Development

No branches or pull requests

2 participants