Offer an option to specify a cache for conditionally fetching public keys

[francisfuzz]

Summary

Closes #9

Previously, clients who fetched verification keys from the server had no way to cache those keys and reuse them for other requests.

This PR proposes a change using the GitHub API's conditional requests feature: clients can optionally specify a cache for their keys and only fetch new ones if the cache is outdated.

BREAKING CHANGE: verifyRequestByKeyId() now returns an object with aisValidproperty and acache` property.

Before

const isValid = await verifyRequestByKeyId();

After

const { isValid, cache } = await verifyRequestByKeyId();

BREAKING CHANGE: fetchVerificationKeys() now returns an object with a keys property and acache property.

Before

const keys = await fetchVerificationKeys();

After

const { keys, cache } = await fetchVerificationKeys();

[francisfuzz]

@gr2m - Thank you for all of your preliminary feedback in draft 🙇

I wrote the last test and left this comment about it for awareness. Happy to make any changes to get this over the line, especially around the way I've defined the types and the tests! 👂

[francisfuzz]

@gr2m - Question on scope for documentation: should I/we update the PR to include the new "state" of things since this is a breaking change? Or, do you prefer documentation-specific changes to land separately in their own PR? 💭

[gr2m]

@gr2m - Question on scope for documentation: should I/we update the PR to include the new "state" of things since this is a breaking change? Or, do you prefer documentation-specific changes to land separately in their own PR

very good call out. As we are changing behavior, we have to update the README.md file where the current API is documented as part of this pull request

[francisfuzz]

I can get on it Monday!

[francisfuzz]

Leaving an update: I've updated the README.md with most of what's changed.

I'll have a final look over when I'm back online. ✌️

[francisfuzz]

@gr2m - This is ready for re-review! I've updated the README, incorporated your suggested feedback in today's most recent review, and re-ran the tests locally to ensure everything passes. I appreciate your time and can make any changes as needed to get this out (or, hold until after tomorrow's stream 😉 ).

[gr2m]

I don't think this is working yet.

Here is a test script

// .gregor/conditional-requests.js
import { request } from "@octokit/request";

import { fetchVerificationKeys } from "../index.js";

let counter = 0;

const requestWithLog = request.defaults({
  request: {
    hook(request, options) {
      counter += 1;
      return request(options);
    },
  },
});

const cache = await fetchVerificationKeys({ request: requestWithLog });
await fetchVerificationKeys({ request: requestWithLog, cache });

console.log({ counter });

This script currently logs out

{ counter: 2 }

But it should log out

{ counter: 1 }

[gr2m]

Ignore me, my script was incorrect, this one is correct

import { request } from "@octokit/request";

import { fetchVerificationKeys } from "../index.js";

const requestWithLog = request.defaults({
  request: {
    async hook(request, options) {
      try {
        const response = await request(options);
        console.log("no cache hit");
        return response;
      } catch (error) {
        if (error.status === 304) {
          console.log("cache hit");
          throw error;
        }

        console.log("unexpected error");
        throw error;
      }
    },
  },
});

const cache = await fetchVerificationKeys({ request: requestWithLog });
await fetchVerificationKeys({ request: requestWithLog, cache });

And the output is as expected

no cache hit
cache hit

[github-actions[bot]]

:tada: This PR is included in version 5.0.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: