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

recycled query teardown #740

Merged
merged 6 commits into from
Jun 2, 2017
Merged

recycled query teardown #740

merged 6 commits into from
Jun 2, 2017

Conversation

jbaxleyiii
Copy link
Contributor

@jbaxleyiii jbaxleyiii commented Jun 1, 2017

Fix for #718, #667, and probably a lot of other loading issues by removing the current subscription before recycling the observable

I'm going to add a test in the morning for this, but for all those impacted, this will be released tomorrow after the flow PR is released in AC. 👍

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

@jbaxleyiii jbaxleyiii changed the title 780 718 Jun 1, 2017
@jbaxleyiii jbaxleyiii changed the title 718 recycled query teardown Jun 1, 2017
@jbaxleyiii jbaxleyiii requested a review from helfer June 1, 2017 04:31
@jbaxleyiii jbaxleyiii self-assigned this Jun 1, 2017
Copy link
Contributor

@helfer helfer left a comment

Choose a reason for hiding this comment

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

We should make sure to check that this still does what the recycling was intended for. If I'm not mistaken, the observable might actually be torn down as soon as it has zero subscribers, which is why the recycler adds a subscriber of its own. Do we have tests for the recycling behavior?

@jbaxleyiii
Copy link
Contributor Author

@helfer we do have tests, but I'm going to work to improve them some. I think this change isn't the full fix we want and may require apollo-client changes. With this change, on remount you get a loading state then the data. Ideally it shouldn't pass loading state.

One question for thought though:
If you contents from your query, is the query loading? If data means no loading, then we could return loading unless we have the data in which case we could set loading to false

@jbaxleyiii
Copy link
Contributor Author

This will fail until apollo-client PR is approved and released

@jbaxleyiii jbaxleyiii merged commit ddd3d8f into master Jun 2, 2017
@jbaxleyiii jbaxleyiii deleted the 780 branch June 2, 2017 12:13
@munyirik
Copy link

@jbaxleyiii could you point me to the commit or PR in apollo-client that coincided with this fix? I'm running into #667 on an apollo-client fork. I'm hoping I can apply the same fix.

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.

None yet

3 participants