apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.38k stars 2.66k forks source link

AutoCleanedWeakCache causes failure when running tests in fakeAsync zone #11790

Closed vz-tl closed 6 months ago

vz-tl commented 7 months ago

Issue Description

Issue occurs in Angular 17 project while executing a Jest test running in fakeAsync zone, imported from @angular/core/testing (pseudo-code):

it('executes ApolloClient in a fakeAsync zone', fakeAsync(() => {
  TestBed.inject(ApolloTestingController).expectOne(...).flush(...);
}));

Result: test fails with Error: 2 timer(s) still in the queue.

Packages:

"@angular/core": "17.3.4",
"@apollo/client": "3.9.0"
"apollo-angular": "6.0.0",
"jest": "29.7.0"

Offending code is located in utilities.cjs :

var scheduledCleanup = new WeakSet();
function schedule(cache) {
    if (!scheduledCleanup.has(cache)) {
        scheduledCleanup.add(cache);
        setTimeout(function () {
            cache.clean();
            scheduledCleanup.delete(cache);
        }, 100); <-- this timeout causes the error in async tests
    }
}
var AutoCleanedWeakCache = function (max, dispose) {
    var cache = new caches.WeakCache(max, dispose);
    cache.set = function (key, value) {
        schedule(this);
        return caches.WeakCache.prototype.set.call(this, key, value);
    };
    return cache;
};

The issue is unrelated to apollo-angular, this package is just being used as a wrapper around ApolloClient (ApolloTestingController used in the test is part of it).

Downgrading @apollo/client to v3.8.10, solves the issue!

Link to Reproduction

-

Reproduction Steps

No response

@apollo/client version

3.9.0+

alessbell commented 7 months ago

Hi @vz-tl 👋 Thanks for the report here.

I was able to reproduce this by wrapping this test in the apollo-angular repo with fakeAsync.

The setTimeout in schedule auto-schedules cleanup of a particular cache (an AutoCleanedWeakCache) when a new item is added, and is throttled to once per 100ms. Clearly this additional microtask is incompatible with your test, but I hope you'll agree it's doing important work :) I'll cc: @phryneas there for any more context he'd like to add.

To unblock your upgrade to 3.9, you can call flush with flush(1) (or flush(2) - not being able to see your test it's hard to say what's causing both) at the end of your test so that only a single microtask is drained from the queue (if there are any more remaining your test will still fail).

I hope that explanation is helpful - I'll leave this open for now in case anyone else on the team has something to add.

phryneas commented 7 months ago

For background - the purpose of this timeout is to batch cleanups of "memoized cache" cleans. If your cache is full and you add 20 more items, you don't want to call .clean() 20 times, but only once, with a little bit of a delay.

That said, until now we scheduled these always, not only when the cache was full. I've opened #11792. Since your tests are unlikely to fill these caches, that should fix it for your usecase.

Could you please test the PR release and report back if it fixes your problem?

npm i @apollo/client@0.0.0-pr-11792-20240419093048
vz-tl commented 7 months ago

@phryneas , with your PR release tests are now working flawlessly!

Thx a lot, @phryneas, @alessbell! You are awesome! Looking forward to pull in the fix release!

github-actions[bot] commented 6 months ago

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

github-actions[bot] commented 5 months ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. For general questions, we recommend using StackOverflow or our discord server.