fix: Fixes type-widening bug when handling plugins. #53
Conversation
|
I think the |
|
Yeah, so it currently silently supports an array - no error or deprecation message will be shown. This PR doesn't have any breaking changes. I can add a deprecation notice and how to migrate to the new syntax in this PR if you would like. |
|
Ah got it. Can you leave one test that keeps testing passing an array? |
| @@ -342,7 +342,7 @@ See [before-after-hook](https://github.com/gr2m/before-after-hook#readme) for | |||
|
|
|||
| ## Plugins | |||
|
|
|||
| Octokit’s functionality can be extended using plugins. THe `Octokit.plugin()` method accepts a function or an array of functions and returns a new constructor. | |||
| Octokit’s functionality can be extended using plugins. The `Octokit.plugin()` method accepts a plugin (or many) and returns a new constructor. | |||
There was a problem hiding this comment.
Maybe we should add a note specifically to TypeScript users. Something like this:
Octokit.plugin() also accepts a single array parameter but we recommend against it, as it breaks the TypeScript/Intellisense support
There was a problem hiding this comment.
The or many part is meant to mean plugins applied in the .plugin(p1, p2, ...) format - not an array. I've updated the syntax throughout the document to use the new, non-array, syntax here and here.
Additionally, there is now a warning logged to the console when the method is supplied with an array.
This should be enough so as to not address TypeScript users specifically - as all users (js and ts) will now be nudged into the correct syntax.
| return { baz: "daz" }; | ||
| } | ||
| ]); | ||
| it("supports array of plugins and warns of deprecated usage", () => { |
There was a problem hiding this comment.
In reference to #53 (comment),
We have kept the test using an array of plugins. In this test, we're also checking for the deprecation notice too.
There was a problem hiding this comment.
Sorry I forgot about it. Thanks for pointing it out
MaximDevoir
changed the title
|
🎉 This PR is included in version 2.4.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
BREAKING CHANGE: `.plugin([plugin1, plugin2])` is now `.plugin(plugin1, plugin2)` (compare octokit/core.js#53, thanks @wolfy1339)
From the ideas that have been thrown out there to solve the issue described in #51, this is my favorite.
Instead of
.plugin(arrayOfPlugins[]), which could lead to a type-widening error, this PR moves to use.plugin(plugin1, plugin2, plugin3, etc...), where each plugin is hardcoded into the type-systemWhy this approach?
I think this will be easiest on plugin authors, who don't have to change anything with how they currently develop plugins - such as having to worry about whether they need to return an object, if they return void, etc.
This will also be an easy fix for users of Octokit.
.plugin([paginateRest, queueWorker, logService])becomes.plugin(paginateRest, queueWorker, logService)TODOs
Example warning when using old syntax:
Fixes #51