Skip to content

On Upgrade to Angular 7, get a tslint error for truthiness check#8968

Closed
navneetkarnani wants to merge 3 commits intoswagger-api:masterfrom
navneetkarnani:patch-2
Closed

On Upgrade to Angular 7, get a tslint error for truthiness check#8968
navneetkarnani wants to merge 3 commits intoswagger-api:masterfrom
navneetkarnani:patch-2

Conversation

@navneetkarnani
Copy link
Copy Markdown

@navneetkarnani navneetkarnani commented Dec 3, 2018

Check these issues for a trail:
microsoft/TypeScript#26262
microsoft/TypeScript@14d3c69

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

The Typescript 3.1 compiler (one that is used with Angular 7 onwards), has a check for void functions being checked for Truthiness. This change fixes the service function to have multiple return types if the HttpClient is being used, which is where the code gets generated.

@jcollard
Copy link
Copy Markdown

jcollard commented Dec 5, 2018

👍 Resolve my issue.

@navneetkarnani
Copy link
Copy Markdown
Author

Addresses: #8836

@navneetkarnani
Copy link
Copy Markdown
Author

@roni-frantchi , since you are the creator of typescript-angular, please review. I don't see a reviewer for typescript on the technical committee

@cbetrifork
Copy link
Copy Markdown

cbetrifork commented Dec 21, 2018

Will the PR not create error when formParams is FormData type, for 'multipart/form-data'?

Since it forces the formParams to be HttpParams or void.

@roni-frantchi
Copy link
Copy Markdown

roni-frantchi commented Dec 23, 2018

@navneetkarnani to @cbetrifork 's point, it seems like your solution holds up because TypeScript isn't smart enough (yet?) to warn you of truthiness checks in void union types.
As in:

interface Foo {
    yo(): number;
    append(): void;
    voidi(): void;
}

interface Bar {
    yo(): string;
    append(): boolean;
}

let union: Foo | Bar;
let foo: Foo;

const unionAsExpected = union.yo() / 10; // Error: you can't divide something that might be a string
const voidAsExpected = foo.voidi() || true; // Error: Void can't be tested for truthiness

const bummer = union.append() || true; // No error... Should have been caught? append() returns a type that's a union of void, can it be checked for truthiness?

Specific data types ( void, HttpParams, etc ) result in better type checking, but also end up being not covering all cases at this time. Any allows working around that problem.
@navneetkarnani
Copy link
Copy Markdown
Author

Will the PR not create error when formParams is FormData type, for 'multipart/form-data'?

Since it forces the formParams to be HttpParams or void.

Changing recommendation to "any" instead of specific type to avoid having to specify all possible combinations ( and that I may not be aware of ).

@roni-frantchi
Copy link
Copy Markdown

roni-frantchi commented Dec 24, 2018

@navneetkarnani using any would solve it, but as you've mention, we lose type safety.
Here's another way to go at it, which I think will solve it: #9012 .
What do you think?.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants