-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
fix(typescript): missing override
directives / satisfy noImplicitOverride
#19896
Conversation
@@ -37,7 +37,7 @@ export class BaseAPIRequestFactory { | |||
* @extends {Error} | |||
*/ | |||
export class RequiredError extends Error { | |||
name: "RequiredError" = "RequiredError"; | |||
override name: "RequiredError" = "RequiredError"; |
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.
public body: ResponseBody, | ||
httpStatusCode: number, | ||
headers: Headers, | ||
body: ResponseBody, | ||
public data: T, |
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.
see
openapi-generator/modules/openapi-generator/src/main/resources/typescript/http/http.mustache
Lines 228 to 230 in 38dac13
public httpStatusCode: number, | |
public headers: Headers, | |
public body: ResponseBody |
override
directivesoverride
directives / satisfy noImplicitOverride
modules/openapi-generator/src/main/resources/typescript/http/http.mustache
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/typescript/http/http.mustache
Show resolved
Hide resolved
@joscha can you re-generate the samples so we can merge? |
Will do. What are your thoughts on tightening the tsconfig? |
@macjohnny this is ready to 🚢 |
In Deno v2 a generator which used to work with v1.x errors with:
due to
noImplicitOverride
(Ensure overriding members in derived classes are marked with an override modifier) being enabled by default.The first one is due to: https://github.com/microsoft/TypeScript/blob/b8e4ed8aeb0b228f544c5736908c31f136a9f7e3/src/lib/es5.d.ts#L1057-L1061
The other three in
http.ts
are simple, as all three properties are already public on the superclass and thus don't need to be exported again:openapi-generator/modules/openapi-generator/src/main/resources/typescript/http/http.mustache
Lines 228 to 230 in 38dac13
There is currently no fixture in the repository capturing these changes. It might be worth adding a change to a sample, so these changes will show up as diff in the futureAlternatively we could make the tsconfig more strict, which would be a good thing in general I think? @macjohnny WDYT?
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming7.x.0
minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)