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 operation dispatching allowing nested operations #356

Merged
merged 1 commit into from
Jul 30, 2019

Conversation

kitten
Copy link
Member

@kitten kitten commented Jul 30, 2019

Fix #345

We run the entire exchange pipeline synchronously,
since that's predictable, which is how Wonka's
sources work.

When an exchange dispatches an operation, for
instance with reexecuteOperation, this operation
is hence run immediately, which can be confusing
and lead to state that disagrees with itself,
since the operation is essentially then "nested".

Instead what we want is to queue operations up
if they're dispatched nestedly, so from inside
an exchange, and flush this queue after the
original dispatch has completed.

We run the entire exchange pipeline synchronously,
since that's predictable, which is how Wonka's
sources work.

When an exchange dispatches an operation, for
instance with reexecuteOperation, this operation
is hence run immediately, which can be confusing
and lead to state that disagrees with itself,
since the operation is essentially then "nested".

Instead what we want is to queue operations up
if they're dispatched nestedly, so from inside
an exchange, and flush this queue after the
original dispatch has completed.
@JoviDeCroock
Copy link
Collaborator

I implemented this fix into the sandbox and this does not seem to execute a network request just yet. Judging through the debugExchange it does go through passed the deduplication but does not go to fetch.

https://codesandbox.io/s/cool-ishizaka-9k4pf

@kitten
Copy link
Member Author

kitten commented Jul 30, 2019

@JoviDeCroock Just tested it in your sandbox 👍 the modification seems to actually be working correctly as far as I can tell. Your sandbox was just missing the actual change in client.js 😅

Edit: here's your sandbox with this PR's changes added to client.js https://codesandbox.io/s/ancient-voice-wvwrm

@JoviDeCroock
Copy link
Collaborator

Yes, codesandbox didn't save my change for some reason. Not getting lucky with my statements today. Just going to approve this. 😄

Copy link
Collaborator

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, fix can be seen working here: https://codesandbox.io/s/cool-ishizaka-9k4pf

@kitten kitten merged commit df6e090 into master Jul 30, 2019
@kitten kitten deleted the fix/dispatch-queueing branch July 30, 2019 12:58
@parkerziegler
Copy link
Contributor

Woah this is really interesting, thanks for the explanation @kitten. So if I'm understanding correctly, the main issue for #345 came down to the fact that, when the requestPolicy was set to "cache-and-network", the cacheExchange is actually calling that reexecuteOperation function, which sends the same operation (by key that is, but with requestPolicy set to "network-only") into the pipeline. This is the "nested operation" case you describe. The solution then is to queue all operations coming into the exchange and flush them once the original, non-nested operation has completed. Does this feel like a fair summary? Just want to make sure I really understand this, since this issue sent me spinning for a little while.

@kitten
Copy link
Member Author

kitten commented Jul 30, 2019

@parkerziegler Yes, that's a fair summary. Since reexecuteOperation eventually calls directly into dispatching the operation on the subject, which would then run through the exchanges synchronously as part of reexecuteOperation's call stack even.

The solution then is to queue all operations coming into the exchange and flush them once the original, non-nested operation has completed

Yep, that's the whole solution. If we're already processing an operation (i.e. are in the exchange pipeline) then we queue up the operation and flush them synchronously after the original one completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cache-and-network seems to not fire the network
3 participants