Improve types for ThrottlingOctokitOptions

[oscard0m]

Description

To improve typing for octokitOptions for plugin-throttling

Context

It can be interesting for:

• deprecating properties/methods
• auto-complete

[oscard0m]

A loooong time without touching TS. If I don't do this Union the compiler does not allow me to use ThrottlingOptions in a place where OctokitOptions is expected.

Doing this union we are not making options.throttle mandatory which is not good.

I still have to play around, maybe with generics OctokitOptions<T>?

[wolfy1339]

Looks good.

Left some comments

[wolfy1339]

We can get better types for Bottleneck. They just aren't published to npm for some reason.

We'll have to vendor them in the repo.

https://github.com/SGrondin/bottleneck/blob/master/light.d.ts

As for the options type, I'm not entirely sure.
The only idea, I have is to check if the throttle property is present and that might satisfy TS

[oscard0m]

We can get better types for Bottleneck. They just aren't published to npm for some reason.

We'll have to vendor them in the repo.

https://github.com/SGrondin/bottleneck/blob/master/light.d.ts

As for the options type, I'm not entirely sure. The only idea, I have is to check if the throttle property is present and that might satisfy TS

I had to create a TS Playground to explain the problem:

Basically the [option: string]: any inside type OctokitOptions is preventing TypeScript to get the branch for type ThrottlingOptions when the shape of throttling is wrong.

Any ideas on how to bypass this? Would this imply to rethink type OctokitOptions? @wolfy1339 (@G-Rath, master of TS, I invoke you 🌋 )

[G-Rath]

If I understand what you're wanting correctly (which is that when plugin-throttling is being used, GithubOptions should have an additional property throttle for options for this plugin), I think you should be using Declaration Merging.

Roughly:

interface OctokitOptions {
    authStrategy?: any;
    auth?: any;
    userAgent?: string;
    previews?: string[];
    baseUrl?: string;
    log?: {
        debug: (message: string) => unknown;
        info: (message: string) => unknown;
        warn: (message: string) => unknown;
        error: (message: string) => unknown;
    };
    request?: any //OctokitTypes.RequestRequestOptions;
    timeZone?: string;
    [option: string]: unknown
};

interface OctokitOptions {
  throttle?: ThrottlingOptions;
}

interface ThrottlingOptions {
  enabled?: boolean;
  Bottleneck?: any;
  id?: string;
  timeout?: number;
  connection?: any;
  onAbuseLimit: LimitHandler;
  onRateLimit: LimitHandler;
}

See playground

This requires that the type be an interface (and in general, why it's generally better for libraries to use interfaces rather than type as it allows downstream consumers to enhance the types), so it'll need a change in @octokit/core.

Once that change has been made, the final code will be something like this:

declare module '@octokit/core' {
  interface OctokitOptions {
    throttle?: ThrottlingOptions;
  }
}

interface ThrottlingOptions {
  enabled?: boolean;
  Bottleneck?: any;
  id?: string;
  timeout?: number;
  connection?: any;
  onAbuseLimit: LimitHandler;
  onRateLimit: LimitHandler;
}

(module pathing can be a bit funny, so that might need adjusting but it's easier to do when the interface actually exists in the package - in the meantime this is a good example of this in action)

[oscard0m]

Types are getting more complex than expected, it will get me some time to sort them out. I'll keep you posted next week

Pushed a second iteration of the types. Still having problems with

plugin-throttling.js/test/octokit.ts

Line 37 in 054cefa

export const TestOctokit = Octokit.plugin(testPlugin, throttling);

I get the following TS error:

Argument of type 'typeof throttling' is not assignable to parameter of type 'OctokitPlugin'.
  Types of parameters 'options' and 'options' are incompatible.
    Type 'OctokitOptions' is not assignable to type '{ throttle: ThrottlingOptionsBase & SecondaryLimitHandler; }'.
      Types of property 'throttle' are incompatible.
        Type 'ThrottlingOptions' is not assignable to type 'ThrottlingOptionsBase & SecondaryLimitHandler'.
          Type 'ThrottlingOptionsBase & AbuseLimitHandler' is not assignable to type 'ThrottlingOptionsBase & SecondaryLimitHandler'.
            Property 'onSecondaryRateLimit' is missing in type 'ThrottlingOptionsBase & AbuseLimitHandler' but required in type 'SecondaryLimitHandler'.ts(2345)
[types.ts(24, 3): ]()'onSecondaryRateLimit' is declared here.

Any clue on what could be the issue? @wolfy1339 @G-Rath

💡 The change in @octokit/core.js to migrate type OctokitOptions to an interface failed 💣 , until the issue is fixed, you will need to replace it manually in your local node_modules to run the tests.

[oscard0m]

Blocked by octokit/core.js#451 resolution to update @octokit/core dependency with latest interface OctokitOptions change (octokit/core.js#450)

I want to re-run checks

[gr2m]

Great PR, thanks for adding type checks!

[github-actions]

🎉 This PR is included in version 3.6.2 🎉

The release is available on:

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

Your semantic-release bot 📦🚀