Problem/Motivation
Provide the option for a cache.
Using a cache, data should not need to be fetched more than once from Drupal. Data should be stored in a local cache (or store). Data fetched from Drupal should be cached and subsequent calls for that data should serve from the cache.
Steps to reproduce
Proposed resolution
This client won't cache things by default, but it should accept a caching mechanism as an option that will be used to cache requests.
Consider locale - might need one cache per locale
I (Brian) really like the Next for Drupal client's API for this, so what I'm proposing here would be very similar to that as a starting point.
The options object will accept a cache, which is an object with:
* a get method that takes a key as a parameter.
* a set method that takes a key and a value as a parameter.
getCollection will also accept an optional cache key, which will be used when getting a collection with cache enabled. With a cache provided, getCollection will first check the cache (based on the provided cache key) and return data from the cache if it exists. If it does have to fetch from the API, it will store the result in the cache for future re-use.
> Consider locale - might need one cache per locale
This doesn't specifically address locale, but it is also an API that is flexible enough to support multiple locales.
To implement this for the POC, we should also choose a cache library to use as an example (an example, not a direct dependency of the client). I'm familiar with Zustand due to using it with Drupal State and think it would be compatible with the proposed API. But open to other viable alternatives - we're trying to prove that this could work generically.
Remaining tasks
* Implement the necessary changes
** Add cache option to apiClient class
** Add cacheKey option to getCollection on the jsonApiClient.
** If cache is provided, check cache before making a fetch request.
** If cache is provided, store any fetched data in cache.
* Define necessary types
* Document the current ApiClient class inline.
* Add test coverage.
User interface changes
API changes
* The options object will now include a cache property.
* getCollection will now accept a cacheKey property.
Data model changes
Issue fork api_client-3377144
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
coby.sher commentedComment #3
brianperryComment #4
brianperryComment #5
coby.sher commentedSmall implementation detail: we should consider a Map() instead of a plain Object. I have not used these in the past myself but I've seem them recently and they have a lot of benefits to a plain object including get and set methods and being iterable by default.
Objects are allowed as keys which could possibly help with the locale issue by saving the locale + the key in a object as the key.
Comment #6
coby.sher commentedDigging into this more. Next.js for Drupal uses node-cache as their default, will probably use that. Open to suggestions if anyone has a strong opinion. A Map() might work but there are probably some lightweight options out there worth using instead.
Comment #7
coby.sher commentedI have an implementation of this mostly done. I tried using Zustand for the example but there's no simple way to maintain the store between page refreshes, so for the current example I used localStorage. We could use indexedDB instead, but either way I like the idea of using the browser for the simple vite example.
It would still be helpful to show an example with Zustand or something similar in an SSR or SPA context but it would involve adding another example app to the examples.
Is that out of scope here or is it worth adding a simple vite+react app to the examples?
Comment #9
coby.sher commentedComment #10
coby.sher commentedComment #11
pratik_kambleComment #12
pratik_kambleI've tested the MR on my local environment and verified that it utilizes the cache from local storage once the cache option is passed to client.
Comment #13
coby.sher commentedAfter rebasing this branch with the latest canary, the tests for the cache feature are failing. Looks like the Spies are no longer being called and I'm not really sure why. They were working previously. I will continue trying to resolve this but if anybody has any ideas or better ways to test this functionality I'm all ears :)
Comment #14
coby.sher commentedOK Fixed the test issue then ran into some strange mismatch of esbuild versions between tsup and the esbuild-node-pollyfill-plugin. @pratik_kamble helped me resolve it – Still unsure what the underlying cause was but to fix it I copied over a pnpm lockfile from a more recent branch and went from there.
This branch should now be up-to-date with the latest canary and ready for another round of review.
Comment #15
pratik_kambleRetested MR on my local environment and verified that it utilizes the cache from local storage once the cache option is passed to client.
Comment #16
brianperrySetting back to needs review since I made a few small tweaks.
Comment #17
pratik_kambleComment #19
brianperryComment #20
brianperryMerged. Thanks @coby.sher and @pratik_kamble!