We should have default strategies for caching

@MattIPv4

As pointed out by @MattIPv4 (https://openjs-foundation.slack.com/archives/C05TRU8PZL6/p1699990976027789) we should be explicit about caching strategies.

And we should also:

Only cache 2XX responses, and if they're get Requests (explicit cache)
Our default responses (on the constants) should have explicit headers for no-cache/no-store
5XX responses also should not have cache
We should extract many other implicit constants as mentioned fix: return a 400 for malformed URIs #64 (comment) to proper constant files

We should extract many other implicit constants ... to proper constant files

Do we want to just move the object to a constants file like this

// src/constants/r2Prefixes.ts
export function getUrlToBucketPathMap(url) {
  return {
    dist: DIST_PATH_PREFIX + (url.pathname.substring('/dist'.length) || '/'),
    download:
      DOWNLOAD_PATH_PREFIX +
      (url.pathname.substring('/download'.length) || '/'),
    docs: DOCS_PATH_PREFIX + (url.pathname.substring('/docs'.length) || '/'),
    api: API_PATH_PREFIX + (url.pathname.substring('/api'.length) || '/'),
    metrics: url.pathname.substring(1), // substring to cut off the /
  }
}

// src/util.ts
const urlToBucketPathMap = getUrlToBucketPathMap(url);
// ...
} else if (basePath in urlToBucketPathMap) {
  bucketPath = urlToBucketPathMap[basePath];
}

or can we do something like this, where we only construct the url we need rather than all of them

// src/constants/r2Prefixes.ts
export function constructDistPath(url) {
  return DIST_PATH_PREFIX + (url.pathname.substring('/dist'.length) || '/');
}
// constructDownloadPath, constructDocsPath, ...

// src/util.ts
} else {
  switch (basePath) {
    case 'dist': {
      bucketPath = constructDistPath(url)
      break;
    }
    // download, docs, ...
    default: {
      if (env.DIRECTORY_LISTING === 'restricted') {
        return undefined;
      }
      bucketPath = url.pathname.substring(1);
    }
  }
}
...

or was there something else you were thinking about?

fix: caching

// src/constants/r2Prefixes.ts
export function getUrlToBucketPathMap(url) {
  return {
    dist: () => `${DIST_PATH_PREFIX}${url.pathname.substring('/dist'.length) || '/'}`,
    download: () => `${DOWNLOAD_PATH_PREFIX}${url.pathname.substring('/download'.length) || '/'}`,
    ...
  }
}

// src/util.ts
const urlToBucketPathMap = getUrlToBucketPathMap(url);
// ...
} else if (basePath in urlToBucketPathMap) {
  bucketPath = urlToBucketPathMap[basePath];
}

I think it's important to mention that we have numerous constant files and it would make sense to have similar constants grouped together (ie. assume we have multiple constants for dist related things or download/ related things, it makes sense to have all these paths and logic to be close together

Is there a reason behind why dist: () => `${DIST_PATH_PREFIX}${url.pathname.substring('/dist'.length) || '/'}`, instead of dist: DIST_PATH_PREFIX + (url.pathname.substring('/dist'.length) || '/'),?

// src/constants/r2Prefixes.ts
const BUCKET_TO_PATHNAME_MAP = {
  dist: ({ pathname }) => `${DIST_PATH_PREFIX}${pathname.substring('/dist'.length) || '/'}`,
  download: ({ pathname }) => `${DOWNLOAD_PATH_PREFIX}${pathname.substring('/download'.length) || '/'}`,
  ...
};

// ...
} else if (basePath in BUCKET_TO_PATHNAME_MAP) {
  bucketPath = BUCKET_TO_PATHNAME_MAP[basePath](url);
}

The main reasoning behind is: You don't calculate and do all these string interpolation at once...

Do we want to tell clients to locally cache HEAD requests? It's what we're doing now, but given we don't cache them on the worker's end I wanted to double check

release-cloudflare-worker/src/handlers/strategies/serveFile.ts

Line 159 in 830d72a

'cache-control': cacheControl,

fix: caching

Navigation Menu

Repository navigation

We should have default strategies for caching #73

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

We should have default strategies for caching #73

Description

Activity

flakey5 commented on Nov 15, 2023

ovflowd commented on Nov 20, 2023

ovflowd commented on Nov 20, 2023

flakey5 commented on Nov 20, 2023

ovflowd commented on Nov 20, 2023

ovflowd commented on Nov 20, 2023

flakey5 commented on Nov 21, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions