Skip to content
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

Support for compilation and usage with strict typescript compiler options. #154

Open
Santalov opened this issue Jul 12, 2022 · 1 comment
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@Santalov
Copy link

Motivation: strict:true is enabled by default in Angular and React projects and considered to be a good choice.

Problem: it is hard to use the generated code with strict:true, although it compiles fine. The main reasons are:

  1. Types of getters and setters don't allow undefined and null in fields containing nested messages. Trying to assign undefined to such fields results in compilation errors and using the getter's value requires an additional check. With primitive fields, Map and Set it works fine after fix: correctly handle defaults for proto2 and proto3 #146 . Related to the issue Fields can be undefined but return type of getters can't #80
  2. The return type of the toObject method has primitive fields, Map and Set fields declared as optional (with ?: preceding a type). So for the typescript it looks like there might be undefined, which leads to unnecessary checks.

The solution is to add | undefined | null to the getter return type and the setter argument when needed. And to remove ? from the declaration of a primitive field, Map or Set field in the toObject method.

Those two reasons result in some tests not compiling under tsconfig strict:true. The next step after solving issues above might be to support strict:true explicitly by specifying it in the tests.
image

Moreover, it would be great if the generated code was compilable with any tsconfig. Looking through all the tsconfig options we can derive the strictest possible one:

  "compilerOptions": {
    "strict": true,
    "exactOptionalPropertyTypes": true,
    "noFallthroughCasesInSwitch": true,
    "noImplicitOverride": true,
    "noImplicitReturns": true,
    "noPropertyAccessFromIndexSignature": true,
    "noUncheckedIndexedAccess": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true
  },

The noImplicitOverride is already supported, but some others don't.

The noUncheckedIndexedAccess:true leads the this.#one_of_decls[0] type to include undefined. This causes the code below to compile with error TS2345: Argument of type 'number[] | undefined' is not assignable to parameter of type 'number[]'. Type 'undefined' is not assignable to type 'number[]'..

set quantity(value: boolean) {
    pb_1.Message.setOneofField(this, 11, this.#one_of_decls[0], value);
}

Solution: add a non-null assertion operator introduced in typescript 2.0 so the indexed access looks like this this.#one_of_decls[0]!

The indexed access also causes a oneof getter to return string | undefined, but undefined actually is never returned. The fix is the same - ! operator.

@Santalov Santalov changed the title Suppor compilation and usage with strict typescript compiler options. Support for compilation and usage with strict typescript compiler options. Jul 12, 2022
@Santalov
Copy link
Author

Santalov commented Jul 12, 2022

I'd like to contribute. My plan is to make bazel build use tsconfig.json file for tests, so we can use the extends option, then fix the issues listed above and make most of the tests use the strictest possible tsconfig (except the noImplicitOverride option).

UPD: No need to use tsconfig.json files, bazel supports the extends option, but at least one tsconfig.json file is required for IDE hints to work properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants