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

Fixed issue with multiple polling queries #262

Merged
merged 5 commits into from
Jun 4, 2016
Merged

Fixed issue with multiple polling queries #262

merged 5 commits into from
Jun 4, 2016

Conversation

Poincare
Copy link
Contributor

@Poincare Poincare commented Jun 2, 2016

Since we were using a single timer reference, there were polling leaks when we had multiple polling queries - see #248

This changeset should fix that issue and introduces a unit test that used to fail but now doesn't.

TODO:

  • Update CHANGELOG.md with your change
  • 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

@zol zol added the in progress label Jun 2, 2016
@@ -94,7 +94,7 @@ export class QueryManager {
private networkInterface: NetworkInterface;
private store: ApolloStore;
private reduxRootKey: string;
private pollingTimer: NodeJS.Timer | any; // oddity in typescript
private pollingTimers: {[queryId: string]: NodeJS.Timer | any};
Copy link
Contributor

Choose a reason for hiding this comment

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

Remind me, why does this have to have an any? If it's an oddity in typescript, maybe we should keep that explanation.

Copy link
Contributor Author

@Poincare Poincare Jun 2, 2016

Choose a reason for hiding this comment

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

I don't really know why. I will find out and add in an explanation.

@helfer
Copy link
Contributor

helfer commented Jun 2, 2016

This is great! Just wondering if we can cut the time necessary for the test.

@jbaxleyiii
Copy link
Contributor

@Poincare this is a great improvement

@helfer helfer added fixed and removed in progress labels Jun 3, 2016
@helfer
Copy link
Contributor

helfer commented Jun 4, 2016

@Poincare Can you rebase this? As soon as it's rebased, we can merge it, and then rebase & merge #251

Awesome to see so many great PRs this week!

@Poincare
Copy link
Contributor Author

Poincare commented Jun 4, 2016

Rebased.

@helfer helfer merged commit 3e0b5c0 into master Jun 4, 2016
@helfer helfer removed the fixed label Jun 4, 2016
@stubailo stubailo deleted the polling_fix branch September 20, 2016 03:43
jbaxleyiii pushed a commit that referenced this pull request Oct 17, 2017
…bject

added docs for default dataIdFromObject selector
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023
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.

4 participants