Fix #647 - Avoid store reading errors on mutations after a failed/canceled query #651
Conversation
|
I don't see how to pass AppVeyor here, something seems broken even before this PR, am I right ? |
|
rebased, appveyor passes = yay! |
|
rebased! @stubailo are you could considering merging that soon? this bug hurts us a lot lately, I can make changes if you need me to do them. Thanks! |
There was a problem hiding this comment.
@rricard this seems more like a workaround than an actual fix, so I'm a bit reluctant to merging it. I'm not sure about it, but to me it seems like it would be more appropriate to check the query state and then return null or skip the updateQueries all together if the query was cancelled or is still in flight and partialData is false. Although in that case we'd have to attempt to queue up the updates and apply them later, I guess.
In the end, this all comes down to how we want to do offline support. Offline support is very important, but it's also not trivial to implement because of all the weird states you can get into.
|
@rricard can you work off of your own fork for now? I'd rather wait until we have a real fix than patch things up half way. |
|
See also #487, which seems to be the same error. |
|
Since this is a pretty serious bug imo, we'll make sure it gets fixed soon! |
|
Also, how does this relate to #511, which you also filed? That sounds pretty similar to me as well. |
|
@helfer whoops! forgot about this one! Closing it! |
|
@helfer in general, working on a fork of the client is a huge pain, we did it once and we're not doing it again, I think (because you also need to fork react-apollo and last time we had to do that, we couldn't just get it out of git like this, we had to recompile + republish under a new name on npm...). I really really prefer working on my own personal time on this to help you refactor all of this because the bug itself is really hard on us (cancellation is the biggest issue I think, fails after errored queries just made us make the schema more stable, which was a good thing). |
|
In general, I agree, that's not the kind of fix I like too, for me the idea was more about providing a test and an early fix. Then we refactor that ASAP but with less pressure since the bug is not there, the test maintaining it down. I think, as discussed in #615, that by simplifying the query diffing we can get it right. |
|
Also the solution you are proposing seems good, I didn't thought about offline support and I have to say that's a complex problem. I'll see tomorrow if I can get a better fix without refactoring the whole thing! |
| } catch (e) { | ||
| if (!/can't find field/i.test(e.message)) { | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
@helfer I agree, this feels completely hacky now that I'm coming back here
|
@helfer or anyone affected: I'm getting the patched version like this in the meantime, not perfect, but will do the trick:
{
"scripts": {
"postinstall": "cd ./node_modules/apollo-client; npm i; npm run prepublish"
},
"dependencies": {
"apollo-client": "apollostack/apollo-client#fix-647"
}
}Note that doing this will slow down pretty much everything... (CI, deploy, etc...) |
|
And... no it doesn't work, tsconfig gets npmignored |
|
Removing the NPM ignore for now so I can use it as a fork! |
|
@rricard ok, I'll try to fix this soon. It might take until next week though, because we'll be away from tomorrow until Monday. |
We avoid running the reducer on those cases Fixes #647
This allows using this branch as an npm fork! TODO: revert this commit
|
@helfer don't worry, either I'll find some time to propose a better fix, either I'll wait, take your time away, enjoy it and don't think about this bug! 👍 |
|
Closing this one: followup in #696 |
Fixes #647
TODO:
Post-mutation failing store reading during updateQueries.