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.

Sign up for GitHub

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

Jump to bottom

Case conversion pipes do not compose under strict TS checking #36259

Closed
ranma42 opened this issue Mar 26, 2020 · 3 comments
Closed

Case conversion pipes do not compose under strict TS checking #36259

ranma42 opened this issue Mar 26, 2020 · 3 comments
Labels
area: common Issues related to APIs in the @angular/common package freq1: low state: confirmed state: has PR type: bug/fix workaround1: obvious
Milestone
Backlog

Comments

@ranma42
Copy link
Contributor

ranma42 commented Mar 26, 2020 •
edited

🐞 bug report

Affected Package

@angular/common
The issue seems to be related to an incoherence between the implementation and signature of LowerCasePipe, TitleCasePipe, and UpperCasePipe.

angular/packages/common/src/pipes/case_conversion_pipes.ts

Lines 32 to 38 in f8096d4

transform(value: string): string {
if (!value) return value;
if (typeof value !== 'string') {
throw invalidPipeArgumentError(LowerCasePipe, value);
}
return value.toLowerCase();
}

Is this a regression?

No, this works as intended when either

  • templates are not typechecked
  • typescript is not strict

Long ago Angular performed very limited checking of the templates, so it was much harder to get to this issue.

Description

The lowercase (/titlecase/uppercase) pipe is implemented to handle null values (which might come from an async or a date pipe), but its signature does not reflect this.

🔬 Minimal Reproduction

https://github.com/ranma42/async-lowercase

🔥 Exception or Error

ERROR in src/app/app.component.html:1:4 - error TS2345: Argument of type 'string | null' is not assignable to parameter of type 'string'.
  Type 'null' is not assignable to type 'string'.

1 {{ v | date | lowercase }}
     ~~~~~~~~

  src/app/app.component.ts:7:16
    7   templateUrl: './app.component.html',
                     ~~~~~~~~~~~~~~~~~~~~~~
    Error occurs in the template of component AppComponent.

🌍 Your Environment

Angular Version:

Angular CLI: 9.1.0
Node: 12.16.0
OS: darwin x64

Angular: 9.1.0
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router
Ivy Workspace: Yes

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.901.0
@angular-devkit/build-angular     0.901.0
@angular-devkit/build-optimizer   0.901.0
@angular-devkit/build-webpack     0.901.0
@angular-devkit/core              9.1.0
@angular-devkit/schematics        9.1.0
@ngtools/webpack                  9.1.0
@schematics/angular               9.1.0
@schematics/update                0.901.0
rxjs                              6.5.4
typescript                        3.8.3
webpack                           4.42.0
@JoostK JoostK added the area: common Issues related to APIs in the @angular/common package label Mar 26, 2020
@ngbot ngbot bot added this to the needsTriage milestone Mar 26, 2020
@ranma42 ranma42 changed the title Case conversion pipes do compose under strict TS checking Case conversion pipes do not compose under strict TS checking Mar 26, 2020
@JoostK
Copy link
Member

JoostK commented Mar 26, 2020

Tracking as FW-2032

@ngbot ngbot bot modified the milestones: needsTriage, Backlog Mar 26, 2020
@ranma42
Copy link
Contributor Author

ranma42 commented Mar 27, 2020

I can write a PR to fix it and possibly improve the typing of other pipes, but I will need some guidance regarding what should happen for undefined values. Some pipes seem to pass them through (slice), some map them to null (date, number).
I also found some inconsistencies in typing (and between typing and implementation), such as async, which is declared as returning undefined when passed undefined, but actually returns null.

What is the best way forward? Opening a PR and discussing this stuff on it?

ranma42 added a commit to ranma42/angular that referenced this issue Mar 27, 2020
@ranma42
fix(common): let case conversion pipes accept type unions with null (…
04c0cf6 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflcts the behaviour on `null` and `undefined`.

Fixes angular#36259
@ranma42 ranma42 mentioned this issue Mar 27, 2020
Closed
14 tasks
ranma42 added a commit to ranma42/angular that referenced this issue Mar 27, 2020
@ranma42
fix(common): let case conversion pipes accept type unions with null (…
fc49edf 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflcts the behaviour on `null` and `undefined`.

Fixes angular#36259
ranma42 added a commit to ranma42/angular that referenced this issue Mar 27, 2020
@ranma42
fix(common): let case conversion pipes accept type unions with null (…
793d570 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflcts the behaviour on `null` and `undefined`.

Fixes angular#36259
ranma42 added a commit to ranma42/angular that referenced this issue Jun 3, 2020
@ranma42
fix(common): let case conversion pipes accept type unions with null (…
265d31c 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflcts the behaviour on `null` and `undefined`.

Fixes angular#36259
ranma42 added a commit to ranma42/angular that referenced this issue Jun 3, 2020
@ranma42
fix(common): let case conversion pipes accept type unions with null (…
3f7c772 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflcts the behaviour on `null` and `undefined`.

Fixes angular#36259
ranma42 added a commit to ranma42/angular that referenced this issue Jun 4, 2020
@ranma42
fix(common): let case conversion pipes accept type unions with null (…
91328a8 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflcts the behaviour on `null` and `undefined`.

Fixes angular#36259
@ranma42 ranma42 mentioned this issue Jun 4, 2020
Closed
14 tasks
kyliau pushed a commit to ranma42/angular that referenced this issue Jun 5, 2020
@ranma42 @kyliau
fix(common): let case conversion pipes accept type unions with null (…
f74242a 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflcts the behaviour on `null` and `undefined`.

Fixes angular#36259
ranma42 added a commit to ranma42/angular that referenced this issue Jun 25, 2020
@ranma42
fix(common): let case conversion pipes accept type unions with null (…
4c40c5e 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflcts the behaviour on `null` and `undefined`.

Fixes angular#36259
ranma42 added a commit to ranma42/angular that referenced this issue Sep 11, 2020
@ranma42
fix(common): let case conversion pipes accept type unions with null (…
9316764 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflcts the behaviour on `null` and `undefined`.

Fixes angular#36259
ranma42 added a commit to ranma42/angular that referenced this issue Sep 20, 2020
@ranma42
fix(common): let case conversion pipes accept type unions with null (…
8f1be78 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflcts the behaviour on `null` and `undefined`.

Fixes angular#36259

BREAKING CHANGE:
The case conversion pipes no longer let falsy values through. They now
map both `null` and `undefined` to `null` and raise an exception on
invalid input (`0`, `false`, `NaN`) just like most "common pipes". If
your code required falsy values to pass through, you need to handle them
explicitly.
AndrewKushnir pushed a commit to ranma42/angular that referenced this issue Sep 20, 2020
@ranma42 @AndrewKushnir
fix(common): let case conversion pipes accept type unions with null (…
551cc70 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflcts the behaviour on `null` and `undefined`.

Fixes angular#36259

BREAKING CHANGE:
The case conversion pipes no longer let falsy values through. They now
map both `null` and `undefined` to `null` and raise an exception on
invalid input (`0`, `false`, `NaN`) just like most "common pipes". If
your code required falsy values to pass through, you need to handle them
explicitly.
ranma42 added a commit to ranma42/angular that referenced this issue Sep 23, 2020
@ranma42
fix(common): let case conversion pipes accept type unions with null (…
b35fd0c 
…angular#36259)

The old implementation of case conversion types can handle several
values which are not strings, but the signature did not reflect this.

The new one reports errors when falsy non-string inputs are given to
the pipe (such as `false` or `0`) and has a new signature which
instead reflects the behaviour on `null` and `undefined`.

Fixes angular#36259

BREAKING CHANGE:
The case conversion pipes no longer let falsy values through. They now
map both `null` and `undefined` to `null` and raise an exception on
invalid input (`0`, `false`, `NaN`) just like most "common pipes". If
your code required falsy values to pass through, you need to handle them
explicitly.
@alxhub alxhub closed this as completed in c7d5555 Sep 28, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area: common Issues related to APIs in the @angular/common package freq1: low state: confirmed state: has PR type: bug/fix workaround1: obvious
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@petebacondarwin @JoostK @ranma42