Implemented a fix for the setTimeout unsubscribe thing #1567
Conversation
Poincare
changed the title
|
@Poincare I'm a bit hesitant merging this, but we could release it under a |
|
Safari: Chrome: 🎉 🎉 |
|
Ok, remind me again, what was the baseline? |
src/core/ObservableQuery.ts
Outdated
|
|
||
| // Stop the query within the QueryManager if we can before | ||
| // this function returns. | ||
| const selectedObservers = that.observers.filter((obs: Observer<ApolloQueryResult<T>>) => obs !== observer); |
There was a problem hiding this comment.
Is this filter here really necessary, or could we just check if the length is 1?
There was a problem hiding this comment.
This filter is necessary due to some race condition that another part of the code mentions. I'm essentially matching the code in tearDownQuery.
|
Mean time for the baseline on Chrome, the browser we were having problems with, was 192.4 ms (see here). |
| next(result) { | ||
| resolve(result); | ||
|
|
||
| // Stop the query within the QueryManager if we can before | ||
| // this function returns. |
There was a problem hiding this comment.
I think we should add more comments about why we need this.
There was a problem hiding this comment.
Thanks @Poincare, that's a pretty nice performance improvement!
I'm a bit worried that we don't have any documentation (in comments or otherwise) about how the result() function should be used, but looking at the code this shouldn't change the behavior of the function, so I think we can merge it.
Should hopefully be a fix for the #1409 and some other related issues.
Took me a while to work out what kinds of tests broke and it's pretty likely that this change breaks behavior that we don't have tests for so we should release it with fingers crossed :)
TODO: