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

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

zbigg
Copy link
Contributor

@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 force-pushed the local-cache-control branch from f460ecd to 338a08d Compare November 28, 2024 15:01
@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
Contributor 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.

@zbigg zbigg force-pushed the local-cache-control branch from 338a08d to 4b0c421 Compare November 28, 2024 15:12
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 71
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, i would go in other direction.

We clearly have "domain" options (url params, post params) and meta params: cache, max url length, context, signal in near future hopefully.

I would separate those "meta" params into one object, so it's easy to pass it around between layers. Those spread operators tend to be tedious when you have to repeat them with more than handful of values in many places.
(look at old react-widgets model props).

Choose a reason for hiding this comment

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

Fair enough

@zbigg zbigg force-pushed the local-cache-control branch from d9b5c79 to 8bf459a Compare December 2, 2024 17:35
@zbigg zbigg force-pushed the local-cache-control branch from 8bf459a to 7c11e42 Compare December 10, 2024 13:53
@zbigg zbigg requested a review from donmccurdy December 10, 2024 15:03
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.

Looks good to me, thanks!

@zbigg zbigg merged commit 5681c7c into main Dec 11, 2024
4 checks passed
@donmccurdy donmccurdy deleted the local-cache-control branch December 12, 2024 15:05
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