cache.set does not resolve with value being cached

[mgmolisani]

Description

When using the cache in plugins, calling cache.set always resolves to undefined when it appears the intention was to have it resolve with what was stored (pretty standard practice).

Steps to reproduce

Attempt to access the return value of await cache.set('a', 'b') in your gatsby-node.js file inside of an api. Mine was in the resolve of a new field in exports.setFieldsOnGraphQLNodeType.

Expected result

For the above code, the expected response is 'b'

based on the content in cache.js:

  set(key, value, args = {}) {
    return new Promise(resolve => {
      this.cache.set(key, value, args, (_, res) => resolve(res))
    })
  }

it should resolve with the response of what was stored. It appears the cache-manager package callback argument suggests a resolve value exists (see: cache-manager). This is what is used under the hood for cache.set but one level deeper into the 'async' package (see: async), we see the third argument callback only gets passed an error or nothing, never a second result argument. I think aync.each is like Array.forEach where it has no returned array unlike map functions.

So my best guess here, is 'cache-manager' promised a result, but under the hood, it's callback is never defining that result since aync.each provides no args or err to that callback hence it bubbles back up to us as undefined. The Gatsby tests for this only check if the response is thenable, which it does return a Promise that just resolves to undefined.

Actual result

Always undefined. All packages that implement cache await the response to make sure something gets stored with the correct key but then return what they passed to the cache instead of a response from the cache which is dangerous if there was ever a mismatch as subsequent cache.get could all be different than the initial value.

Environment

Got errors running this. I definitely have more than just Edge installed.... I'm not a monster.

  System:
    OS: Windows 10
    CPU: (4) x64 Intel(R) Core(TM) i5-6300U CPU @ 2.40GHz
  Binaries:
    npm: 6.5.0 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: 42.17134.1.0
  npmPackages:
    gatsby: ^2.0.76 => 2.0.76
    gatsby-image: ^2.0.20 => 2.0.25
    gatsby-plugin-manifest: ^2.0.9 => 2.0.12
    gatsby-plugin-offline: ^2.0.16 => 2.0.20
    gatsby-plugin-react-helmet: ^3.0.2 => 3.0.5
    gatsby-plugin-sharp: ^2.0.14 => 2.0.16
    gatsby-source-contentful: ^2.0.26 => 2.0.26
    gatsby-source-filesystem: ^2.0.8 => 2.0.12
    gatsby-source-shopify: ^2.0.8 => 2.0.8
    gatsby-transformer-sharp: ^2.1.8 => 2.1.9

[DSchau]

This seems like a super easy fix to implement, so thanks for identifying!

Would you be interested in resolving the value rather than the res (which is apparently undefined) and additionally adding a test or two to guard against this behavior?

Generally - I don't know why you'd want to retrieve the value of a cache.set, since you already have that value. That being said--I see how is confusing behavior, particularly if there's an implicit return in an async function.

[mgmolisani]

Wow, amazingly fast reply and confirmation. Loving Gatsby so much.

To share on why returning the value is important, I'll relate it to an HTTP POST or PUT request which is similarly asynchronous. Typically when you update asynchronously, you want some confirmation that the data you wrote actually got written and didn't get mutated in any way (unintentionally).

A potentially better example on the same HTTP comparison would be if the server adds some timestamp to the data. You can either send out a new GET request with an ID to get that timestamp info, or the response payload of the POST or PUT can return the newly written/updated object to you with the timestamp it added (the second is far more common for hopefully obvious reasons).

I think from that perspective, this isn't so simple of an issue as it is a problem that seems to live in the chosen cache-manager package. The best we can do on the Gatsby end (aside from ditch the package which is doing most of what it promises) is assume that if no error was returned (the first arg), then our data got cached as we sent it and we can resolve the sent value as you suggested in the callback since the cache should never mutate like a server might to an HTTP request.

Does that make sense? Overall, you are right. It can be easily worked around, but resolving the value that was actually written avoids potential mismatches later.

I'd be happy to create a PR with the suggested change, but I'm pretty green to github collaborating and the open source community are all very new to me. If I have any issue with this, I suppose I'll just refer back to this issue for help?

[DSchau]

If I have any issue with this, I suppose I'll just refer back to this issue for help?

Definitely! We also offer community programming sessions so you and a Gatsby maintainer can work on this (or any!) problem together!

Also some quick notes:

A potentially better example on the same HTTP comparison would be if the server adds some timestamp to the data. You can either send out a new GET request with an ID to get that timestamp info, or the response payload of the POST or PUT can return the newly written/updated object to you with the timestamp it added (the second is far more common for hopefully obvious reasons).

I agree with these, but these examples--while nice!--don't seem strictly related to what we're discussing here. If we were augmenting the value with the cache, e.g. adding a timestamp or a unique key or something--then I can see how this is a huge issue. As it stands, it seems like more of an annoyance, but an annoyance that I'm happy to see pointed out and one that we can fix pretty easily!

One note you made there re: error handling (e.g. if we are just ignoring the error) is concerning. We should probably throw there so our Promise rejects correctly.

[mgmolisani]

I agree. Since we know we aren’t mutating our data in the cache, we can safely return our input. This issue definitely arose out of my own annoyance as you pointed out as this makes 3 lines of code (define, set, return) just an inlined return.

Regarding error handling, the current callback ignores the err arg completely with a ‘_’ so my solution only built off that assumption that the error was insignificant in Gatsby's eyes and we could use it simply as a flag that our data did or did not make it there unmutated. I agree that handling it would also be a good addition if we think it is required.

[wardpeet]

🎉 we have a winner! Your PR got merged!