Octokit: Try upgrading to v19

[ockham]

Description

Spurred by #28424 (comment).

Currently based on #28424; might change that later, or rebase once #28424 has landed.

The Octokit API has changed drastically since v16.*, with most types now being auto-generated from GH's REST API docs, and made available through helpers that basically accept REST API routes as parameters, as described at https://github.com/octokit/types.ts/tree/63fd0dcd31c1a547170ccdb72e7885a9b4080745#examples, e.g.

import { Endpoints } from "@octokit/types";

type listUserReposParameters = Endpoints["GET /repos/{owner}/{repo}"]["parameters"];
type listUserReposResponse = Endpoints["GET /repos/{owner}/{repo}"]["response"];

async function listRepos(
  options: listUserReposParameters
): listUserReposResponse["data"] {
  // ...
}

Alas, that kind of type doesn't seem to work too well with JSDoc: While it seems that I've managed to get the example into a shape that's accepted by TS, JSDoc throws a number of syntax errors 😕

The alternative (which is based on octokit instances) works inside of a context where such an instance is available -- but can't be used where it isn't, e.g. in a function's @return type declaration.

import {
  GetResponseTypeFromEndpointMethod,
  GetResponseDataTypeFromEndpointMethod,
} from "@octokit/types";
import { Octokit } from "@octokit/rest";

const octokit = new Octokit();
type CreateLabelResponseType = GetResponseTypeFromEndpointMethod<
  typeof octokit.issues.createLabel
>;
type CreateLabelResponseDataType = GetResponseDataTypeFromEndpointMethod<
  typeof octokit.issues.createLabel
>;

[gr2m]

I cannot help with the JSDoc errors I'm afraid.

[aduth]

Kinda seems like inferring types might be the most reasonable option here? 😬

Re: JSDoc, might just need to /* eslint-disable jsdoc/valid-types */ 🤷 Pending improved support.

Can't recall what all repercussions might be involved, but wonder if it might be worth giving up on jsdoc/valid-types altogether and disable it project-wide.

[aduth]

There should only be a single value returned from this, hence an error about mismatched types on how listMilestonesResponse["data"] is an array. I guess an alternative could be to pull in the type for the singular item response?

/** @typedef {Endpoints["GET /repos/{owner}/{repo}/milestones/{milestone_number}"]["response"]} getMilestoneResponse */

Then:

Suggested change

	* @return {Promise<listMilestonesResponse["data"]|undefined>} Promise resolving to milestone, if exists.
	* @return {Promise<getMilestoneResponse["data"]|undefined>} Promise resolving to milestone, if exists.

Or, reference the base types schema:

/** @typedef {import('@octokit/openapi-types').components["schemas"]["milestone"]} Milestone */

Or maybe it's not worth documenting it, and just letting the inferred type go through 🤷

Suggested change

* @return {Promise<listMilestonesResponse["data"]|undefined>} Promise resolving to milestone, if exists.

[ockham]

There should only be a single value returned from this, hence an error about mismatched types on how listMilestonesResponse["data"] is an array.

Thanks! I got a bit caught up in the JSDoc syntax error and missed that this one was actually (also) a TS problem.

Thanks for your suggested fixes! I think I'll go with the Milestone (from @octokit/openapi-types, which I was unaware of), since it's the most semantic option.

Or maybe it's not worth documenting it, and just letting the inferred type go through 🤷

I seem to be getting any in that case though 🤔 (looking at the call site in changelog.js).

[ockham]

Hey @aduth, thanks for dropping by! 😄

Kinda seems like inferring types might be the most reasonable option here?

Going with explicit for now -- I felt like I should provide a @return type for those functions.

Re: JSDoc, might just need to /* eslint-disable jsdoc/valid-types */ Pending improved support.

Yeah, I was hoping to avoid that, but looks like a reasonable trade-off 👍

Can't recall what all repercussions might be involved, but wonder if it might be worth giving up on jsdoc/valid-types altogether and disable it project-wide.

Indeed, might be worth considering 🤔

[ockham]

Seems like TS is complaining since it expects a string | undefined here -- but the docs say that a number should be okay 🤔

[ockham]

This would indeed fix the issue, but I think we might want to report this upstream instead:

diff --git a/bin/plugin/lib/milestone.js b/bin/plugin/lib/milestone.js
index 226d968980..7fe20cfeee 100644
--- a/bin/plugin/lib/milestone.js
+++ b/bin/plugin/lib/milestone.js
@@ -43,7 +43,7 @@ async function getMilestoneByTitle( octokit, owner, repo, title ) {
  * @param {GitHub}     octokit       Initialized Octokit REST client.
  * @param {string}     owner         Repository owner.
  * @param {string}     repo          Repository name.
- * @param {number}     milestone     Milestone ID.
+ * @param {string}     milestone     Milestone ID.
  * @param {IssueState} [state]       Optional issue state.
  * @param {string}     [closedSince] Optional timestamp.
  *

[gr2m]

I think we might want to report this upstream instead

This is indeed a bug in GitHub's OpenAPI spec, from which all the @octokit types and method names are generated:

      - name: milestone
        description: If an `integer` is passed, it should refer to a milestone by
          its `number` field. If the string `*` is passed, issues with any milestone
          are accepted. If the string `none` is passed, issues without milestones
          are returned.
        in: query
        required: false
        schema:
          type: string

I filed an issue here: github/rest-api-description#226

[ockham]

Wow, beat me to it! Thanks a lot @gr2m!