We should have default strategies for caching

[ovflowd]

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 https://github.com/nodejs/release-cloudflare-worker/pull/64#issuecomment-1808057972 to proper constant files

[flakey5]

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?

[ovflowd]

// 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];
}

[ovflowd]

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

[flakey5]

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) || '/'),?

[ovflowd]

// 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);
}

[ovflowd]

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

[flakey5]

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 https://github.com/nodejs/release-cloudflare-worker/blob/830d72a0c9096da3fb46280fbaa7a493d5d65db4/src/handlers/strategies/serveFile.ts#L159