Skip to content

Conversation

Montana
Copy link

@Montana Montana commented Sep 4, 2025

Hey all,

The truthiness bug I also fixed, if (cacheHit) failed for cached values like 0, "", or false. Using Map + .has() makes cache hits reliable.if (cacheHit) failed for cached values like 0, "", or false. Using Map + .has() makes cache hits reliable.

At the end of the day it still calls:

const cache = require('./fixtureCache')(Promise);

cache.set('foo', 123);
cache.get('foo'); // → 123

cache.getOrPromise('bar', () => fetchStuff())
  .then((val) => console.log(val));

Cheers,
Michael

Copy link

qodo-merge-pro bot commented Sep 4, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Promise Ctor Consistency

Ensure all Promise usages consistently use the injected constructor; verify that any external calls (e.g., setAsync timing) and returned promises interoperate correctly with environments that pass a custom Promise implementation.

module.exports = function (P) {
  const PromiseCtor = P || Promise;

  class FixtureCache {
    constructor() {
      this.cache = new Map();
      this.inflight = new Map();
    }

    set(key, value) {
      this.cache.set(key, value);
    }

    get(key) {
      return this.cache.get(key);
    }

    getOrPromise(key, fn) {
      if (this.cache.has(key)) {
        return PromiseCtor.resolve(this.cache.get(key));
      }

      if (this.inflight.has(key)) {
        return this.inflight.get(key);
      }

      const p = PromiseCtor.resolve()
        .then(fn)
        .then((result) => {
          this.setAsync(key, result);
          this.inflight.delete(key);
          return result;
        })
        .catch((err) => {
          this.inflight.delete(key);
          throw err;
        });

      this.inflight.set(key, p);
      return p;
    }

    setAsync(key, value) {
      setTimeout(() => this.set(key, value), 0);
    }
Inflight Cleanup

Confirm inflight entries are always removed even if fn throws synchronously or returns a non-thenable; also validate behavior when fn resolves to undefined/falsy results and caching semantics remain correct.

const p = PromiseCtor.resolve()
  .then(fn)
  .then((result) => {
    this.setAsync(key, result);
    this.inflight.delete(key);
    return result;
  })
  .catch((err) => {
    this.inflight.delete(key);
    throw err;
  });

this.inflight.set(key, p);
return p;
Async Set Timing

setAsync uses setTimeout with a 0ms delay without a clear need; verify consumers don’t depend on synchronous cache population and consider microtask scheduling to preserve ordering with custom Promise implementations.

setAsync(key, value) {
  setTimeout(() => this.set(key, value), 0);
}

Copy link

qodo-merge-pro bot commented Sep 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent race in deduplication

Deleting inflight before the async cache write introduces a race window where
subsequent calls can miss both cache and inflight, causing duplicate executions.
Set the cache synchronously before deleting inflight to eliminate this gap. This
preserves deduplication guarantees under concurrent access.

src/fixtures/FixtureCache.js [29-33]

 .then((result) => {
-        this.setAsync(key, result);
+        // Set cache synchronously to avoid a gap that can trigger duplicate inflight requests
+        this.set(key, result);
         this.inflight.delete(key);
         return result;
       })
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a race condition where a key is removed from inflight before being added to cache due to setAsync. This could cause duplicate executions, defeating the purpose of the inflight map. The proposed fix is critical for correctness.

High
  • More

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

Successfully merging this pull request may close these issues.

1 participant