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

Build failing due to octokit plugin-rest-endpoint #39712

Closed
nonara opened this issue Jul 23, 2020 · 3 comments
Closed

Build failing due to octokit plugin-rest-endpoint #39712

nonara opened this issue Jul 23, 2020 · 3 comments

Comments

@nonara
Copy link

nonara commented Jul 23, 2020

Note:

Filing an issue here as well so that it can be tracked if anyone else experiences the same.

Steps:

git clone https://github.com/microsoft/TypeScript.git
cd TypeScript
npm install
npm run build:compiler

Search Terms: octokit, build failing

Affected Versions: 3.9 - 4.0 fail (3.8.2 succeeded)

Errors

[21:44:13] Finished 'generateLibs' after 571 ms
node_modules/@octokit/plugin-rest-endpoint-methods/dist-types/generated/parameters-and-response-types.d.ts:961:60 - error TS2339: Property 'GET /user/:migration_id/repositories' does not exist on type 'Endpoints'.

961             parameters: RequestParameters & Omit<Endpoints["GET /user/:migration_id/repositories"]["parameters"], "baseUrl" | "headers" | "mediaType">;
                                                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/@octokit/plugin-rest-endpoint-methods/dist-types/generated/parameters-and-response-types.d.ts:962:33 - error TS2339: Property 'GET /user/:migration_id/repositories' does not exist on type 'Endpoints'.

962             response: Endpoints["GET /user/:migration_id/repositories"]["response"];
                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/@octokit/plugin-paginate-rest/dist-types/generated/paginating-endpoints.d.ts:987:31 - error TS2339: Property 'GET /user/:migration_id/repositories' does not exist on type 'Endpoints'.

987         parameters: Endpoints["GET /user/:migration_id/repositories"]["parameters"];
                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

node_modules/@octokit/plugin-paginate-rest/dist-types/generated/paginating-endpoints.d.ts:988:29 - error TS2339: Property 'GET /user/:migration_id/repositories' does not exist on type 'Endpoints'.

988         response: Endpoints["GET /user/:migration_id/repositories"]["response"];
                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 4 errors.
@nonara
Copy link
Author

nonara commented Jul 23, 2020

Issue fixed on plugin side. octokit/plugin-rest-endpoint-methods.js#172

Forthcoming from contributor is: 'post-mortem to explain what happened, how I'll make sure it won't happen again.'

@nonara nonara closed this as completed Jul 23, 2020
@orta orta mentioned this issue Jul 23, 2020
@gr2m
Copy link

gr2m commented Jul 23, 2020

Here is the promised post-mortem:
octokit/plugin-rest-endpoint-methods.js#172 (comment)

I'll also look through your code base to see how you use @octokit/rest. It's possible that using @octokit/core instead with only the plugins you need might be a better fit. It has a much smaller API surface, which will lower the risk for future incidents as well as lower the maintenance overhead, as there are not nearly as many breaking changes. It also has a smaller bundle size which will fasten your build times

@gr2m
Copy link

gr2m commented Jul 23, 2020

I looked through your code and I think you should use @octokit/core instead of @octokit/rest.

The changes will look like this. Instead of

gh.pulls.get({ pull_number: +process.env.SOURCE_ISSUE, owner: "microsoft", repo: "TypeScript" }))

the code will be

// https://docs.github.com/en/rest/reference/pulls#get-a-pull-request
gh.request('GET /repos/:owner/:repo/pulls/:pull_number', { pull_number: +process.env.SOURCE_ISSUE, owner: "microsoft", repo: "TypeScript" }))

gh.request has TypeScript definitions for all known routes, so from a user experience it's not such a big trade off. @octokit/core is roughly 1/3 of the code size, and it does not need to generate the 600+ rest endpoint methods each time you instantiate an Octokit instance. You also won't be affected by method name changes, which happened a lot in the past (it will slow down now though).

I'd be happy to send a pull request if you'd like that.

One thing I'd like to do first though is to update the type definitions for the paths from /repos/:owner/:repo/pulls/:pull_number to /repos/{owner}/{repo}/pulls/{pull_number}. Both work at run time, but the latter aligns with the new documentation, e.g. https://docs.github.com/en/rest/reference/pulls#get-a-pull-request

I promise to release that change as a breaking change in @octokit/types 🤞🏼😁

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

No branches or pull requests

2 participants