Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Suggestion: Don't return last result, return actual cache data #858

Closed
wants to merge 4 commits into from

Conversation

alexzielenski
Copy link

This is a quick code change suggestion from #801. It brings the code more in line with what the actual comment says is the writer's intent. We will simply re-run the same query on the store, with fresh options.

@apollo-cla
Copy link

@alexzielenski: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@jbaxleyiii
Copy link
Contributor

@alexzielenski this is a great change to be more inline with stock Apollo-client usage! Thank you! Would you be able to write a test so we can verify this is happening?

If you have any questions, I'm happy to help!

@apollo-cla
Copy link

apollo-cla commented Jul 17, 2017

Fails
🚫

No CHANGELOG added.

Warnings
⚠️

Please assign someone to merge this PR, and optionally include people who should review.

⚠️

There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

@alexzielenski
Copy link
Author

alexzielenski commented Jul 17, 2017

@jbaxleyiii No thanks. I identified this as an issue almost a month ago and was brushed off (I suggested this exact fix in my first post but I guess no one really read it) I just got tired seeing this bug in my project. This just seemed like the only way I could possibly express what's wrong with the current implementation to you guys since you were closing my issue. I don't intend to fix any errors or set up a build environment for this. This is it.

@jbaxleyiii
Copy link
Contributor

@alexzielenski I'm sorry to hear that you have a bad experience suggesting improvements to Apollo. The past month has seen some changes to our team and honestly, we have always been small and relied on community support. This entire library was developed by contributors so I really hate to see that you didn't have a positive experience.

I'm happy to write a test case for this and get it into an upcoming release. If there is anything that I can do to improve your experience next time please don't hesitate to let me know.

All the best 👍

@akomm
Copy link
Contributor

akomm commented Jul 24, 2017

This is obviously a buggy behavior and should be fixed. I have another bug, which make this one leak data and there might be other situation where this leads to leaked data.

  1. After authentication user information is loaded via GraphQL. After logout I clear all state from redux (apollo is run in same store and is also cleared -> confirmed via dev tools)

  2. After this user logs out, when another user wants to login an an error is raised while fetching new data, the data from last query is loaded, even though the store was cleared (the other bug: Old query data loaded, even though the redux store was cleared #890).

I'm totally aware, that when apollo gets the data from somewhere, it is also accessible without this behavior. It just makes it visible. But it makes it easier also shows how flawed this behavior is.

update 1

Nevermind. I think the issue is not related with this one, as it also loads cleared data from store without any error.

@stale
Copy link

stale bot commented Aug 14, 2017

This issue has been automatically labled because it has not had recent activity. If you have not received a response from anyone, please mention the repository maintainer (most likely @jbaxleyiii). It will be closed if no further activity occurs. Thank you for your contributions to React Apollo!

@stantoncbradley
Copy link

would this pertain to #807 at all? these cases around logging in and seeing old users data is really concerning

@marlonmleite
Copy link

marlonmleite commented Aug 21, 2017

Hey, in my company have the same problem.
It's bad for user and my QA report many bugs.
It's necessary to do several jobs of client.resetStore() to all code in a way not cause the problem.

This is very bad :(

@stale
Copy link

stale bot commented Sep 12, 2017

This issue has been automatically labled because it has not had recent activity. If you have not received a response from anyone, please mention the repository maintainer (most likely @jbaxleyiii). It will be closed if no further activity occurs. Thank you for your contributions to React Apollo!

@jbaxleyiii jbaxleyiii closed this Sep 22, 2017
@Ardhimas
Copy link

Ardhimas commented Oct 25, 2017

Why was this not merged? It looks like a good PR. @jbaxleyiii

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

Successfully merging this pull request may close these issues.

7 participants