persistor.getSize() and maxSize not supported when serialize is false

[jimbuck]

I'm utilizing a custom JSON serialization configuration so I've set serialize to false and supplied my own storage instance. Unfortunately the built-in Storage class is written to expect a JSON string for calculating size. That means it always returns null if the result is not a string. Which means the auto-purge doesn't work correctly.

apollo-cache-persist/src/Storage.ts

Lines 30 to 38 in 7bcb322

	async getSize(): Promise<number | null> {
	const data = await this.storage.getItem(this.key);
	if (data == null) {
	return 0;
	} else {
	return typeof data === 'string' ? data.length : null;
	}
	}

It would make sense to let storage implementations provide their own getSize function, but I figured I would file this and get some feedback before starting on a PR.

I'm thinking something simple like this:

async getSize(): Promise<number | null> { 
   if (this.storage.getSize) {
      return await this.storage.getSize(this.key);
   }

   const data = await this.storage.getItem(this.key); 
  
   if (data == null) { 
     return 0; 
   } else { 
     return typeof data === 'string' ? data.length : null; 
   } 
 } 

It's optional, backwards compatible, and gives full control over how size is calculated. Any thoughts?

[jimbuck]

So I've tried to implement local work arounds for this, but I've found other areas where changes will need to be made in order to support non-string data. For example, the Persistor class can't purge when the maxSize is reached since the size is calculated again manually (rather than calling getSize()). I will try to put a PR together to address all of this, but not sure about timeline.

[wtrocki]

@jimbuck As you have mentioned this could be a tricky change. For the moment it is easy to say that for serialize false limits will not be enforced. Code we have in repo is not the best as typically we should have storage limits enforced and event that handles storage limit and wipes it out. Calculating size is very heavy operation and totally redundant when indexed db database have native methods to enforce size and handle overflows.

That being said. I think for advanced cases we can recommend to avoid using internal method and implement your own.

[jspizziri]

@jimbuck any thoughts on the previous comment? If you’ve gone a different direction or have found a solution I’ll close this issue.

[jimbuck]

We did come up with a work-around but it just replaces some methods on the persistor and storage instances (this is from 0.10.0):

this.persistor = new CachePersistor({
  cache: this.cache,
  storage: this.localStorage,
  serialize: false,
  maxSize: 4.5 * MEGABYTES,
  debounce: 5 * SECONDS,
});

this.persistor.storage.getSize = async () => {
  return this.localStorage.getSize();
};

// Directly override the persist method to use the getSize method on our localStorage service.
this.persistor.persistor.persist = async () => {
  try {
    if (this.persistor.persistor.paused) return;

    const data = this.cache.extract();
    const size = await this.localStorage.getSize();

    if (size > (this.persistor.persistor.maxSize ?? 0)) {
      console.log(`Purged cache of size ${size} characters`);
      await this.persistor.purge();
      this.persistor.persistor.paused = true;
      return;
    }

    await this.persistor.storage.write(data);

    console.log(`Persisted cache of size ${size} characters`);
  } catch (error) {
    console.error('Error persisting cache', error);
    throw error;
  }
};

this.localStorage is a custom class that includes a dedicated getSize function. It is technically checking the current size of the persisted data, rather than the size of the data to be persisted. We've accounted for this by just lowering the maxSize so it that it still saves but will get purged on the next persist (4.5MB limit when local storage is typically 5MB).

I am trying to put together a PR that will show it more officially implemented. It should be up soon as a better reference.

[jimbuck]

When I created our work-around, that was in 0.10.0, which didn't have the persistenceMapper. I will have to rethink our approach to the maxSize check since it kinda changed how maxSize works at a fundamental level. But the draft PR I created will at least let CachePersistor#getSize work correctly for custom storage instances with a getSize function.