Update types to reflect baseUrl changes #815
Conversation
|
@tcbyrd Yes that should fix it. #803 added a It might be helpful to include all of the optional arguments to the constructor so that they are updated in the TypeScript definition when they are updated in the JavaScript. |
| host?: string; | ||
| pathPrefix?: string; | ||
| protocol?: string; | ||
| port?: number; |
There was a problem hiding this comment.
I think I would leave these, they are deprecated but still supported. That way we don’t break existing code.
Is there a way to mark something as "deprecated" in Typescript?
There was a problem hiding this comment.
I think one way is to decorate it with @deprecated so that you get the message from the language server. Not sure if this causes a warning in the build.
There was a problem hiding this comment.
Mind if I update your PR? I’ve extended the typescript test to make it less likely that I break it again :(
There was a problem hiding this comment.
JSDoc doesn't cause a build failure, but it does do this:
when you do this:
/**
* @deprecated in version 15.2.0
*/
host?: string;
There was a problem hiding this comment.
I don’t use JS doc in this repo, is this an editor plugin?
There was a problem hiding this comment.
is this an editor plugin?
It's part of the VS Code / Atom language server for TypeScript
There was a problem hiding this comment.
okay thanks, I added them to each deprecated option
|
Does the PR look good to you now @philschatz @tcbyrd Btw if you have an app depending on |
|
@gr2m I think once if we finally get Probot switched over, that could be a great way to test. The compiler should help catch things like this. |
Unless @philschatz has any other suggestions on deprecations, I'd say it's good to 🚢 |
|
Yep, the JSDoc comment is the only way I know of to add deprecation notices to TypeScript files. 👍 for merging. |
|
🎉 This PR is included in version 15.2.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@gr2m @philschatz I think this is the change you need to the type definitions to reflect the deprecations in https://github.com/octokit/rest.js/pull/807
See https://github.com/octokit/rest.js/pull/732#issuecomment-373554225 for more context on where I ran into this.