feat(cache): add default param to cache.get()

[dbowling]

• cache.get() gains second parameter, enabling a default value to be defined on first call for a given key

Closes issue: #3276

[dbowling]

@straker I had to spread the params of get() to account for the possibility of an undefined default value. This does change the way the cache behaves a bit though--any params passed in excess of the first were ignored, and passing no params gave you some unexpected behavior.

It could be smart to add a guard to get() and throw if there are the wrong number of args, considering how cache methods can be a real pain if they aren't predictable. Adding a throw, though, feels like it is at a greater risk of introducing a breaking change than adding this feature without.

If we do want to add it, here's the patch:

diff --git a/lib/core/base/cache.js b/lib/core/base/cache.js
index ec414ec6..d0773055 100644
--- a/lib/core/base/cache.js
+++ b/lib/core/base/cache.js
@@ -21,6 +21,10 @@ const cache = {
     const args = [...arguments];
     const hasDefaultValue = args.length === 2;
 
+    if (args.length > 2 || args.length === 0) {
+      throw new Error('incorrect number of arguments');
+    }
+
     if (
       hasDefaultValue &&
       !(key in _cache) &&
diff --git a/test/core/base/cache.js b/test/core/base/cache.js
index cb0befe9..b9f51018 100644
--- a/test/core/base/cache.js
+++ b/test/core/base/cache.js
@@ -96,4 +96,18 @@ describe('axe._cache', function () {
   it('should return undefined for key lookup miss when no default value is provided', function () {
     assert.strictEqual(typeof axe._cache.get('foo'), 'undefined');
   });
+
+  it('should throw if there are too many arguments', function () {
+    function fn() {
+      axe._cache.get('foo', 'bar', 'baz');
+    }
+    assert.throws(fn);
+  });
+
+  it('should throw if there are no arguments', function () {
+    function fn() {
+      axe._cache.get();
+    }
+    assert.throws(fn);
+  });
 });

[straker]

I would argue that it is not valid to set the cache with a value of undefined. undefined is the only way we know if the cache was not set before, so it should not be a valid default argument value either. That is, passing undefined would just look at the cache value without setting a default (passing undefined and passing nothing are equivalent).

[dbowling]

Although passing undefined and passing nothing are equivalent when reading the value directly, they are not equivalent in this PR because of the arg spreading, which can determine the difference between the caller's usage. In one case it writes the key with a value of undefined, and in the other (no second arg passed) it would not set the key at all.

The set() function does not distinguish though. It writes to the cache with no checks.

I would propose that if we consider undefined to not be a valid cache value, we codify that decision in both the get() and set() logic and tests as the current behavior would then be a bug. Probably via a separate issue/PR for the fix, and putting this on-hold until it is merged into develop.

[straker]

I rescind my comment on undefined being a valid value.

[dbowling]

The following files in lib use cache.set(), but I elected not to include them in the refactor

• lib/core/utils/get-flattened-tree.js - only sets new values and does not use return
• lib/core/public/run/globals-setup.js - set needs to overwrite
• lib/checks/generic/page-no-duplicate-evaluate.js - refactor of this would likely be substantially harder to read

[dbowling]

I converted this PR back to draft while I work through the expanded scope of updating lib to use the default value poperty.

[stephenmathieson]

What is the benefit of passing a default value? Why can't callers do:

thing = cache.get('thing') || defaultValue

[dbowling]

Stephen, that gets back to what Wilco put in https://github.com/dequelabs/axe-core/issues/3276.

Your example thing = cache.get('thing') || defaultValue is close, but would need to also set the default value to the cache. In practice, that creates a pattern that us trying to be generalized.

Would this be more clear if we left get() as is, and added the new function call? This is a bit wordy, but gets the discussion started: cache.getAndSetDefaultIfNotSet(key, default)?

[stephenmathieson]

Looks like the issue is asking for:

var thing = cache.pull('thing', () => {
  // This is only run if `thing` is not in the cache.
  return 7
})

This PR is not doing that. The issue also never makes any mention of undefined.

[dbowling]

Yes, that and a CI/local test discrepancy is why I took this back to draft status. If we want to change the intended behavior, thats okay too. Cache is a potential nightmare for bugs to hide, so I am all about being intentional.

The undefined discussion started out of a review of finding appropriate test cases for the change. The code isn't currently explicitly defining how undefined should be treated and it is up to the caller. It appears there are mixed opinions on that, so it's good we are discussing as we evaluate how we want to use the cache.

[dbowling]

@straker or @WilcoFiers - how would you like me to handle the current failed tests for places where the creator is returning undefined and throwing an error?

If we want to continue to throw, I'll need to touch these functions and check before sending, put in try/catch, or add an options object that permits cache to quietly ignore (e.g. not setting a value and not throwing) when it encounters this case.

Happy to do whatever you want, I'm mindful of scope bloat on this PR though. If that scope increase concerns you, we can still remove the throw (or make it a non-default setting) as that is new behavior for this PR.

[dbowling]

The reply to my last comment can be found here.

Since the assert is throwing for is-modal-open, we also decided that a better solution than treating that as a bug is to memoize.