-
Notifications
You must be signed in to change notification settings - Fork 328
fix: Fixes type-widening bug when handling plugins. #53
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
Changes from all commits
8721bb3
95abc09
5f2fd6b
7b72bbd
9ec6862
6b8acd6
19a5680
816fdf9
9f170c5
d90d3b4
ff21ca1
15d41bd
ee41f60
76cc0ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`Octokit.plugin() supports array of plugins and warns of deprecated usage 1`] = ` | ||
[MockFunction] { | ||
"calls": Array [ | ||
Array [ | ||
"Passing an array of plugins to Octokit.plugin() has been deprecated. | ||
Instead of: | ||
Octokit.plugin([plugin1, plugin2, ...]) | ||
Use: | ||
Octokit.plugin(plugin1, plugin2, ...)", | ||
], | ||
], | ||
"results": Array [ | ||
Object { | ||
"type": "return", | ||
"value": undefined, | ||
}, | ||
], | ||
} | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,41 +1,43 @@ | ||
import { Octokit } from "../src"; | ||
|
||
const pluginFoo = () => { | ||
return { foo: "bar" }; | ||
}; | ||
const pluginBaz = () => { | ||
return { baz: "daz" }; | ||
}; | ||
const pluginQaz = () => { | ||
return { qaz: "naz" }; | ||
}; | ||
|
||
describe("Octokit.plugin()", () => { | ||
it("gets called in constructor", () => { | ||
const MyOctokit = Octokit.plugin(() => { | ||
return { | ||
foo: "bar", | ||
}; | ||
}); | ||
const MyOctokit = Octokit.plugin(pluginFoo); | ||
const myClient = new MyOctokit(); | ||
expect(myClient.foo).toEqual("bar"); | ||
}); | ||
|
||
it("supports array of plugins", () => { | ||
const MyOctokit = Octokit.plugin([ | ||
() => { | ||
return { | ||
foo: "bar", | ||
}; | ||
}, | ||
() => { | ||
return { baz: "daz" }; | ||
}, | ||
]); | ||
it("supports array of plugins and warns of deprecated usage", () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I forgot about it. Thanks for pointing it out |
||
const warningSpy = jest.spyOn(console, "warn").mockImplementation(); | ||
const MyOctokit = Octokit.plugin([pluginFoo, pluginBaz]); | ||
const myClient = new MyOctokit(); | ||
expect(myClient.foo).toEqual("bar"); | ||
expect(myClient.baz).toEqual("daz"); | ||
expect(warningSpy).toMatchSnapshot(); | ||
warningSpy.mockClear(); | ||
}); | ||
|
||
it("supports multiple plugins", () => { | ||
const MyOctokit = Octokit.plugin(pluginFoo, pluginBaz, pluginQaz); | ||
const myClient = new MyOctokit(); | ||
expect(myClient.foo).toEqual("bar"); | ||
expect(myClient.baz).toEqual("daz"); | ||
expect(myClient.qaz).toEqual("naz"); | ||
}); | ||
it("does not override plugins of original constructor", () => { | ||
const MyOctokit = Octokit.plugin((octokit) => { | ||
return { | ||
foo: "bar", | ||
}; | ||
}); | ||
const MyOctokit = Octokit.plugin(pluginFoo); | ||
const myClient = new MyOctokit(); | ||
expect(myClient.foo).toEqual("bar"); | ||
|
||
const octokit = new Octokit(); | ||
expect(octokit).not.toHaveProperty("foo"); | ||
}); | ||
|
@@ -64,17 +66,9 @@ describe("Octokit.plugin()", () => { | |
}); | ||
|
||
it("supports chaining", () => { | ||
const MyOctokit = Octokit.plugin(() => { | ||
return { | ||
foo: "bar", | ||
}; | ||
}) | ||
.plugin(() => { | ||
return { baz: "daz" }; | ||
}) | ||
.plugin(() => { | ||
return { qaz: "naz" }; | ||
}); | ||
const MyOctokit = Octokit.plugin(pluginFoo) | ||
.plugin(pluginBaz) | ||
.plugin(pluginQaz); | ||
|
||
const myClient = new MyOctokit(); | ||
expect(myClient.foo).toEqual("bar"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 supportThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.