Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .github/workflows/samples-typescript-typecheck.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
name: TypeScript clients type checks
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This workflow runs the type check shell file


on:
pull_request:
paths:
- samples/**
Copy link
Member

@wing328 wing328 Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joscha can we run this only if there's a change in the typescript-related samples?

#20000 (a recent PR) shows the workflow failed: https://github.com/OpenAPITools/openapi-generator/actions/runs/11606840555/job/32319917402?pr=20000

can you please take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can reduce it a bit more. In order to be super precise we'd need the full dependency graph, that is tricky as it includes any package.lock, node versions, etc. - it's not impossible but the repository is not set up for it currently, we'd want to also get the node dependency from nix for example. I can probably improve it a bit more, however I won't have access to a computer for a few more days. If the tests fail regularly with false positives, feel free to comment out the yaml and I'll fix and reenable when I get access to a computer.

- bin/ts-typecheck-all.sh
- .github/workflows/samples-typescript-typecheck.yaml
jobs:
build:
name: Typecheck TypeScript samples
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we were to generate this file, we could used the samples axis to produce one for each project with tsconfig. This would run the checks in parallel and make failures easier to understand. It would be the first time we do a dynamic build step though. Let me know what you prefer. We can also improve this later.

node-version:
- 20
steps:
- uses: actions/checkout@v4

- uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node-version }}

- name: Run type checker
run: ./bin/ts-typecheck-all.sh
49 changes: 49 additions & 0 deletions bin/ts-typecheck-all.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
#!/usr/bin/env bash

set -euo pipefail

log() {
echo "$@" >&2
}

npm_install() {
# --ignore-scripts because we don't want to run any pre- or postinstall scripts
# --no-package-lock because we don't want to update or create the package-lock.json
# --no-fund because we don't want to check for funding
# --no-audit because we don't want to run an audit
# --suppress-warnings because we don't want to see any warnings whilst type checking
npm i \
--suppress-warnings \
--ignore-scripts \
--no-package-lock \
--no-fund \
--no-audit \
"$@"
}

main() {
local root_dir
root_dir=$(git rev-parse --show-toplevel)
local dir

for dir in $(git ls-files samples | grep 'tsconfig.json$' | xargs -n1 dirname | sort -u); do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We find all checked in samples that have a tsconfig.json

if [[ ! -f "${root_dir}/${dir}/.openapi-generator-ignore" ]]; then
# This is not a generated sample; skip it
continue
fi
if [[ ! -f "${root_dir}/${dir}/package.json" ]]; then
# we can't really guarantee that all dependencies are there to do a typecheck...
continue
fi
log "${dir}"
pushd "${root_dir}/${dir}" > /dev/null
npm_install \
|| npm_install --force # --force because we have some incompatible peer-dependencies that can't be fixed
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can see in the CI logs, but basically there are a couple really really outdated dependencies, which can't even be updated anymore, like https://github.com/angular/tsickle/ for example. Hence we need to sometimes override incompatible peer dependencies, etc.

npm exec [email protected] --yes -- tsc --noEmit
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does the actual type checking and fails the job if there are errors

log "${dir}"
log
popd > /dev/null
done
}

main "$@"
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ public AbstractTypeScriptClientCodegen() {
typeMapping.put("Array", "Array");
typeMapping.put("array", "Array");
typeMapping.put("boolean", "boolean");
typeMapping.put("decimal", "string");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without this change,

for example would exopect a full-blown Decimal class, which is never generated. You can see code references to it that are not resolved as the file is not there. For example:

typeMapping.put("string", "string");
typeMapping.put("int", "number");
typeMapping.put("float", "number");
Expand Down Expand Up @@ -746,6 +747,11 @@ public String toDefaultValue(Schema p) {
return p.getDefault().toString();
}
return UNDEFINED_VALUE;
} else if (ModelUtils.isDecimalSchema(p)) {
if (p.getDefault() != null) {
return p.getDefault().toString();
}
return UNDEFINED_VALUE;
} else if (ModelUtils.isDateSchema(p)) {
if (p.getDefault() != null) {
return p.getDefault().toString();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export class Api {
*/
protected ensureParamIsSet<T>(context: string, params: T, paramName: keyof T): void {
if (null === params[paramName]) {
throw new Error(`Missing required parameter ${paramName} when calling ${context}`);
throw new Error(`Missing required parameter ${String(paramName)} when calling ${context}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes:

Api.ts:73:53 - error TS2731: Implicit conversion of a 'symbol' to a 'string' will fail at runtime. Consider wrapping this expression in 'String(...)'.

73       throw new Error(`Missing required parameter ${paramName} when calling ${context}`);
                                                       ~~~~~~~~~


Found 1 error in Api.ts:73

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,11 @@ function querystringSingleKey(key: string, value: string | number | null | undef
return `${encodeURIComponent(fullKey)}=${encodeURIComponent(String(value))}`;
}

export function exists(json: any, key: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes:

src/models/GetPetRegionsResponse.ts:15:10 - error TS2305: Module '"../runtime"' has no exported member 'exists'.

15 import { exists, mapValues } from '../runtime';
            ~~~~~~

const value = json[key];
return value !== null && value !== undefined;
}

{{^withoutRuntimeChecks}}
export function mapValues(data: any, fn: (item: any) => any) {
return Object.keys(data).reduce(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export function appFromJS(any: any): any {
if (isIndexed(value)) {
return knownIndexedSetByKey.indexOf(key) !== -1 ? value.toSet() : value.toList();
} // we're reviving an array -> it's a List
const MatchingType = knownRecordFactories.get(value.get('recType')) as { new(input?: any): any }; // check if we know a Record with this type
const MatchingType = knownRecordFactories.get(value.get('recType') as string) as { new(input?: any): any }; // check if we know a Record with this type
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes

src/runtimeSagasAndRecords.ts:16:55 - error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'string'.

16         const MatchingType = knownRecordFactories.get(value.get('recType')) as { new(input?: any): any }; // check if we know a Record with this type
                                                         ~~~~~~~~~~~~~~~~~~~~

if (MatchingType) {
return new MatchingType(value);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export function *{{nickname}}Saga() {
yield takeLatest({{nickname}}, {{nickname}}SagaImp);
}

export function *{{nickname}}SagaImp(_action_: Action<Payload{{#lambda.titlecase}}{{#lambda.camelcase}}{{nickname}}{{/lambda.camelcase}}{{/lambda.titlecase}}>) {
export function *{{nickname}}SagaImp(_action_: Action<Payload{{#lambda.titlecase}}{{#lambda.camelcase}}{{nickname}}{{/lambda.camelcase}}{{/lambda.titlecase}}>){{^returnType}}: any{{/returnType}} {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no implicit any

const {markErrorsAsHandled, ..._payloadRest_} = _action_.payload;
try {
{{#returnTypeSupportsEntities}}
Expand Down Expand Up @@ -233,7 +233,7 @@ export function *{{nickname}}SagaImp(_action_: Action<Payload{{#lambda.titlecase
{{^returnType}}
return undefined;
{{/returnType}}
} catch (error) {
} catch (error: any) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no implicit any

if (markErrorsAsHandled) {error.wasHandled = true; }
yield put({{nickname}}Failure({error, requestPayload: _action_.payload}));
return error;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
{{#useAxiosHttpModule}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nestjs/axios is only in the package.json when useAxiosHttpModule is truthy. If not, the package is not a dependency, which then fails in the type checks, as it can't be resolved. Other code uses the HttpService from nestjs/common in that case, so we do that here as well now, too.

import type { HttpService } from '@nestjs/axios';
{{/useAxiosHttpModule}}
{{^useAxiosHttpModule}}
import type { HttpService } from '@nestjs/common';
{{/useAxiosHttpModule}}
import { ModuleMetadata, Type } from '@nestjs/common/interfaces';

export interface ConfigurationParameters {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
"@nestjs/testing": "~{{nestVersion}}",
"@types/express": "^4.16.0",
"@types/jest": "^24.0.15",
"@types/node": "^14.8.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These types are so old that some definitions clash with the current typescript version.

"@types/node": "*",
"@types/supertest": "^2.0.8",
"concurrently": "^4.1.1",
"nodemon": "^1.19.1",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,13 +123,14 @@ export type ApiKeyConfiguration = string;
export type HttpBasicConfiguration = { "username": string, "password": string };
export type HttpBearerConfiguration = { tokenProvider: TokenProvider };
export type OAuth2Configuration = { accessToken: string };
export type HttpSignatureConfiguration = unknown; // TODO: Implement
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not actually sure what this has to look like. Currently it's not supported as we don't have it implemented. This fix is to produce valid syntactic code, the implementation is probably a bit more involved.
You can see the fix it produces below in samples/openapi3/client/petstore/typescript/builds/explode-query/auth/auth.ts.
And how it is currently broken (syntactically invalid TS), here:

"bearer_test"?: HttpBearerConfiguration,
"http_signature_test"?:
}


export type AuthMethodsConfiguration = {
{{^useInversify}}
"default"?: SecurityAuthentication,
{{/useInversify}}
{{#authMethods}}
"{{name}}"?: {{#isApiKey}}ApiKeyConfiguration{{/isApiKey}}{{#isBasicBasic}}HttpBasicConfiguration{{/isBasicBasic}}{{#isBasicBearer}}HttpBearerConfiguration{{/isBasicBearer}}{{#isOAuth}}OAuth2Configuration{{/isOAuth}}{{^-last}},{{/-last}}
"{{name}}"?: {{#isApiKey}}ApiKeyConfiguration{{/isApiKey}}{{#isBasicBasic}}HttpBasicConfiguration{{/isBasicBasic}}{{#isBasicBearer}}HttpBearerConfiguration{{/isBasicBearer}}{{#isOAuth}}OAuth2Configuration{{/isOAuth}}{{#isHttpSignature}}HttpSignatureConfiguration{{/isHttpSignature}}{{^-last}},{{/-last}}
{{/authMethods}}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,12 @@ export class ResponseContext {
return result;
}

const parameters = this.headers[headerName].split(";");
const parameters = this.headers[headerName]!.split(";");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is guarded further up, but some of the older TS versions we have can't determine this, yet.

for (const parameter of parameters) {
let [key, value] = parameter.split("=", 2);
if (!key) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined guard

continue;
}
key = key.toLowerCase().trim();
if (value === undefined) {
result[""] = key;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,8 @@ export class ServerConfiguration<T extends { [key: string]: string }> implements

private getUrl() {
let replacedUrl = this.url;
for (const key in this.variableConfiguration) {
var re = new RegExp("{" + key + "}","g");
replacedUrl = replacedUrl.replace(re, this.variableConfiguration[key]);
for (const [key, value] of Object.entries(this.variableConfiguration)) {
replacedUrl = replacedUrl.replaceAll(`{${key}}`, value);
}
return replacedUrl
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ type MimeTypeDescriptor = {
* the payload.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined guards here too

*/
const parseMimeType = (mimeType: string): MimeTypeDescriptor => {
const [type, subtype] = mimeType.split('/');
const [type = '', subtype = ''] = mimeType.split('/');
return {
type,
subtype,
Expand Down Expand Up @@ -272,7 +272,7 @@ export class ObjectSerializer {
if (mediaType === undefined) {
return undefined;
}
return mediaType.split(";")[0].trim().toLowerCase();
return (mediaType.split(";")[0] ?? '').trim().toLowerCase();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,26 @@ export class {{classname}} {{#parent}}extends {{{.}}} {{/parent}}{
{{/vars}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noImplicitOverride fixes


{{#discriminator}}
static readonly discriminator: string | undefined = "{{discriminatorName}}";
static {{#parent}}override {{/parent}}readonly discriminator: string | undefined = "{{discriminatorName}}";
{{/discriminator}}
{{^discriminator}}
static readonly discriminator: string | undefined = undefined;
static {{#parent}}override {{/parent}}readonly discriminator: string | undefined = undefined;
{{/discriminator}}
{{#hasDiscriminatorWithNonEmptyMapping}}

static readonly mapping: {[index: string]: string} | undefined = {
static {{#parent}}override {{/parent}}readonly mapping: {[index: string]: string} | undefined = {
{{#discriminator.mappedModels}}
"{{mappingName}}": "{{modelName}}",
{{/discriminator.mappedModels}}
};
{{/hasDiscriminatorWithNonEmptyMapping}}
{{^hasDiscriminatorWithNonEmptyMapping}}

static readonly mapping: {[index: string]: string} | undefined = undefined;
static {{#parent}}override {{/parent}}readonly mapping: {[index: string]: string} | undefined = undefined;
{{/hasDiscriminatorWithNonEmptyMapping}}

{{^isArray}}
static readonly attributeTypeMap: Array<{name: string, baseName: string, type: string, format: string}> = [
static {{#parent}}override {{/parent}}readonly attributeTypeMap: Array<{name: string, baseName: string, type: string, format: string}> = [
{{#vars}}
{
"name": "{{name}}",
Expand All @@ -58,7 +58,7 @@ export class {{classname}} {{#parent}}extends {{{.}}} {{/parent}}{
{{/vars}}
];

static getAttributeTypeMap() {
static {{#parent}}override {{/parent}}getAttributeTypeMap() {
{{#parent}}
return super.getAttributeTypeMap().concat({{classname}}.attributeTypeMap);
{{/parent}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
"rxjs": "^6.4.0",
{{/useRxJS}}
{{#useInversify}}
"inversify": "^5.0.1",
"inversify": "^6.0.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inversify 5.x doesn't work properly with our current TS version. It's this issue here: https://stackoverflow.com/questions/75776252/inversify-typescript-5-ts1239-unable-to-resolve-signature-of-parameter-decor

{{/useInversify}}
"es6-promise": "^4.2.4",
"url-parse": "^1.4.3"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { inject, injectable, multiInject, optional, interfaces } from "inversify";

import { Configuration } from "../configuration";
import { ServerConfiguration, servers } from "../servers";
import { ServerConfiguration, servers{{#servers}}, server1{{/servers}} } from "../servers";
import { HttpLibrary{{^useRxJS}}, wrapHttpLibrary{{/useRxJS}} } from "../http/http";
import { Middleware{{^useRxJS}}, PromiseMiddlewareWrapper{{/useRxJS}} } from "../middleware";
import { authMethodServices, AuthMethods } from "../auth/auth";
Expand Down Expand Up @@ -42,7 +42,7 @@ class InjectableConfiguration implements AbstractConfiguration {
public authMethods: AuthMethods = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

services/index.ts:26:9 - error TS2322: Type 'ServerConfiguration<{}> | undefined' is not assignable to type 'AbstractServerConfiguration'.
  Type 'undefined' is not assignable to type 'AbstractServerConfiguration'.

26         @inject(AbstractServerConfiguration) @optional() public baseServer: AbstractServerConfiguration = servers[0],
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

services/index.ts:68:54 - error TS2345: Argument of type 'ServerConfiguration<{}> | undefined' is not assignable to parameter of type 'AbstractServerConfiguration'.
  Type 'undefined' is not assignable to type 'AbstractServerConfiguration'.

68         this.bindServerConfiguration.toConstantValue(servers[idx]);
                                                        ~~~~~~~~~~~~


constructor(
@inject(AbstractServerConfiguration) @optional() public baseServer: AbstractServerConfiguration = servers[0],
@inject(AbstractServerConfiguration) @optional() public baseServer: AbstractServerConfiguration{{#servers}} = server1{{/servers}},
@inject(AbstractHttpLibrary) @optional() httpApi: AbstractHttpLibrary,
@multiInject(AbstractMiddleware) @optional() middleware: AbstractMiddleware[] = [],
@multiInject(AbstractAuthMethod) @optional() securityConfiguration: AbstractAuthMethod[] = []
Expand Down Expand Up @@ -90,6 +90,9 @@ export class ApiServiceBinder {
* return value;
*/
public bindServerConfigurationToPredefined(idx: number) {
if (!servers[idx]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

servers[idx] can be undefined

throw new Error(`Server ${idx} is not available.`);
}
this.bindServerConfiguration.toConstantValue(servers[idx]);
return servers[idx];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,29 @@
"declaration": true,

/* Additional Checks */
"noUnusedLocals": false, /* Report errors on unused locals. */ // TODO: reenable (unused imports!)
"noUnusedParameters": false, /* Report errors on unused parameters. */ // TODO: set to true again
"noImplicitReturns": true, /* Report error when not all code paths in function return a value. */
"noFallthroughCasesInSwitch": true, /* Report errors for fallthrough cases in switch statement. */
"noUnusedLocals": false, /* Report errors on unused locals. */ // TODO: reenable (unused imports!)
"noUnusedParameters": false, /* Report errors on unused parameters. */ // TODO: set to true again
"noImplicitReturns": true, /* Report error when not all code paths in function return a value. */
"noFallthroughCasesInSwitch": true, /* Report errors for fallthrough cases in switch statement. */
"noImplicitOverride": true, /* Ensure overriding members in derived classes are marked with an override modifier. */
"noPropertyAccessFromIndexSignature": true, /* Enforces using indexed accessors for keys declared using an indexed type. */
"noUncheckedIndexedAccess": true, /* Add 'undefined' to a type when accessed using an index. */
Comment on lines +20 to +22
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added these three


"removeComments": true,
"sourceMap": true,
"outDir": "./dist",
"noLib": false,
{{#platforms}}
"lib": [
"es6"
,"ES2017.Object"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brings Object.entries

,"ES2021.String"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String.replaceAll

{{#node}}
"lib": [ "es6" ],
{{/node}}
{{#browser}}
"lib": [ "es6", "dom" ],
,"dom"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unified this, browser only gets "dom" in addition to the rest.

{{/browser}}
],
{{/platforms}}
{{#useInversify}}
"experimentalDecorators": true,
Expand Down
Loading
Loading