Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable control of cache behavior for source and query APIs #33

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zbigg
Copy link

@zbigg zbigg commented Nov 28, 2024

Add cache control parameters to sources and query API.

  • detect caching from HTTP headers.
  • allow selecting cache behavior as well as providing custom cache storage

Usage example:

  // for new users: will obey standard HTTP Control-Cache
  const myCache = new Map()
  const widgetSource = new vectorQuerySource({
    sqlQuery: 'select * from ...',
    // disable local cache, note browser and proxy cache can still cache something
    localCache: { cacheControl: 'no-cache' }
    // to completly prevent caching you must *also* set headers
    headers: { 'Cache-Control: no-cache' },
    // use specific map for cache cache
    localCache: { cache: myCache }
  });
  
  something.onClick() = {
    // clear cache before in context let's say - refresh action
    myCache.clear()
    rerenderLayers()
  }

@zbigg zbigg changed the title source: enable control of cache behavior enable control of cache behavior for source and query APIs Nov 28, 2024
@zbigg
Copy link
Author

zbigg commented Nov 28, 2024

This very raw proposal that solves problem of cache control in most minimalistic way

  • fixes behavior for old-clients that wanted already tried to control cache with headers
  • allows to explictly control local cache behavior

I still left Cache-Control intepretation in this proposal as this was my first attempt. Now i see this may be too far-fetched and too complex and buggy (there are more APIs to control). We should probably remove it and rely only on explict props.

On the other hand current behavior is simply wrong. People may pass Cache-control: no-cache ... and find that request is cached anyway ... Leaving it for consideration.

Copy link
Member

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tempted to stop inspecting headers to determine the local cache settings, and let that be based on the localCache prop alone. Do you think that would be OK? Presumably this would land in deck.gl v9.1.

It is nice that users who need to explicitly clear the cache can now do:

const cache = new Map()
const widgetSource = new WidgetQuerySource({localCache: { cache }, ... });

...

cache.clear();

Comment on lines 64 to 68
export type LocalCacheOptions = {
canReadCache?: boolean;
canStoreInCache?: boolean;
cache?: Map<string, Promise<unknown>>;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming we plan to respect the definitions of HTTP CacheControl headers, do we want to borrow their API too? This could be...

export type LocalCacheOptions = {
  cache?: Map<string, Promise<unknown>>;
  cacheControl?: ('no-cache' | 'no-store')[]
}

... which is nice because external users can find existing API documentation easily.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we are down to two options, I wouldn't use a nested cache object and instead just provide these options directly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants