Implementing initial cache to reduce runtime dependencies

[santino]

Hey @ardatan, I'm starting to look into implementation of initial cache as discussed in #1522.
I have a few questions, I thought that starting a discussion would be best to have a dynamic conversation.

Which cache to use and how

I am playing with cache-file and I see you're using lodash for set and get.
Can I ask why?
Ultimately you're dealing with an object (this.json) that you then write into the file. To set and retrieve object entries you don't need lodash.
Asking this because lodash set has a bad behavior to nest object properties, splitting by "dot". You can see how for a URL this is bad, because any subdomain will incur into a sub-property of the object key.
We cannot set keys like this, cause otherwise when we need to re-populate cache from file at runtime, those keys won't match.
For instance a uri like this: https:domain.subdomain.something.more/myendpoint results in an entry like: { "https:domain": { subdomain: { something: { more: { myendpoint: 'actual cache entry value' } } } } }.
I was tempted to remove lodash but I believe is there for a reason; hence the question.

Also, I considered using the default LRU cache, which exposes dump and load methods.
I could get cache from getMesh and then do cache.dump() and it will contain all entries serialised and ready to be written into file.

Using cache-file I guess makes sense, but the behaviour would need to be corrected.

How to bypass ttl

How can we achieve this?
Ultimately ttl is automatically inferred by fetchache and this doesn't take any option that we can potentially pass.

[ardatan]

You're right! We can use "localStorage" approach like having one-level key-value pairs instead of lodash's for safer caching.
Currently fetchache only respects HTTP caching headers and stuff. Surely we can control this behavior to force it to cache HTTP responses no matter what. It's what we should need for this initial cache stuff. So I agree with you :)

[santino]

Sounds good, I will make the relevant changes.

One other thing that doesn't make me fully happy with going the cache-file approach is the package dependency.
Ultimately the cache is used by @graphql-mesh/config, this is where we set it to transforms, handlers etc.

However, if on CLI command I tell the config to set a file cache, the package for cache-file needs to be available in @graphql-mesh/config.
Effectively for a CLI requirement, we need to add a new dependency to a package other than CLI, in this case@graphql-mesh/config.

One way to potentially solve this is to add a new cache option to findAndParseConfig.
In this case on CLI I can install cache-file as a dependency and pass to findAndParseConfig { cache: new FileCache({ path: './something' } ) }. Then in findAndParseConfig I will have to override config.cache (potentially set by users) with options.cache (set by CLI).
This is not great as it can be confusing to users since it allows them to have another way to register cache and there could be conflicts between config file and findAndParseConfig options.

Another way to avoid this is, as I mentioned above, is to use the default LRU cache, and then dump its value into file.
LRU cache is already a dependency of @graphql-mesh/config and it is actually used by the package, so it doesn't have the risk of being a dependency that the package doesn't apparently do anything with, as cache-file would be.
On my CLI command, I will still have to override any cache potentially set by users in the config file, but in this case, this will happen only on the CLI command without opening the door for users to ambiguous setting of cache in config and/or option.

I think I am personally leaning more towards the LRU + file dump approach.

[santino]

In the meantime I have raised #96 in fetchache.
The last point was an interesting discovery, seems like without these changes we were always issuing an extra fetch request even if cache was valid, just because we didn't set headers to say we accept stale responses.

Might need some thinking, since it needs to be applied to all requests.

[santino]

The approach has been changed to not force caching policies, and be a bit closer to initial proposal #1522.

Eventually, this was introduced in #1820, so this discussion can be marked as resolved