feat(common): stricter types for SlicePipe #30156

cexbrayat · 2019-04-26T19:45:21Z

Adds overloads to the transform methods of SlicePipe,
to have better types than any for value and any as a return.
With this commit, using slice in an ngFor still allow to type-check the content of the ngFor
with fullTemplateTypeCheck enabled in Ivy:

<div *ngFor="let user of users | slice:0:2">{{ user.typo }}</div>
                                                    |
                                                    `typo` does not exist on type `UserModel`

whereas it is currently not catched (as the return of slice is any) neither in VE nor in Ivy.

BREAKING CHANGE
SlicePipe now only accepts an array of values, a string, null or undefined.
This was already the case in practice, and it still throws at runtime if another type is given.
But it is now a compilation error to try to call it with an unsupported type.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: #30086

What is the new behavior?

The slice pipe is now properly typed, so Ivy and fullTemplateTypeCheck enabled will catch errors inside an ngFor using a slice.

Does this PR introduce a breaking change?

Yes
No

Technically this changes the API, as it was previously possible to (wrongly) call slice.transform(14, 1); which still throws at runtime but is now a compile time error.

Other information

@alxhub as we talked about it on Slack

alfaproject · 2019-04-26T20:55:41Z

packages/common/test/pipes/slice_pipe_spec.ts

@@ -28,14 +28,17 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
      it('should support lists', () => { expect(() => pipe.transform(list, 0)).not.toThrow(); });

      it('should not support other objects',
-         () => { expect(() => pipe.transform({}, 0)).toThrow(); });
+         () => { expect(() => pipe.transform({} as string, 0)).toThrow(); });


as any would be less weird

trotyl · 2019-04-27T06:47:28Z

packages/common/src/pipes/slice_pipe.ts

@@ -62,6 +62,10 @@ export class SlicePipe implements PipeTransform {
   *   - **if positive**: return all items before `end` index of the list or string.
   *   - **if negative**: return all items before `end` index from the end of the list or string.
   */
+  transform<T>(value: T[], start: number, end?: number): T[];


I guess it should also accept ReadonlyArray<T>?

Good point. I don't know if it's best to have another overload, or if transform<T, A extends ReadonlyArray<T>>(value: A, start: number, end?: number): A; is enough...

IgorMinar · 2019-04-29T05:54:04Z

if this PR presubmits without any failures in google3 which would confirm the theory that this is not a de facto breaking change then I'd be fine merging this change into the 8.0.x branch.

kara

LGTM. Running presubmit...

kara · 2019-05-07T22:14:47Z

presubmit

kara · 2019-05-07T23:01:00Z

@IgorMinar Looks like this passed TAP in google. Are you good with merging this?

alxhub · 2019-05-07T23:55:19Z

packages/common/src/pipes/slice_pipe.ts

@@ -62,6 +62,10 @@ export class SlicePipe implements PipeTransform {
   *   - **if positive**: return all items before `end` index of the list or string.
   *   - **if negative**: return all items before `end` index from the end of the list or string.
   */
+  transform<T, A extends ReadonlyArray<T>>(value: A, start: number, end?: number): A;


I don't think this is technically correct. ReadonlyArray<T>.slice() returns a T[] not a ReadonlyArray<T>. SlicePipe should do the same - slicing a ReadonlyArray<T> results in a T[].

That is, it should be valid in a template to write:

{{ foo(readOnly | slice:0:3) }}

Even if foo() requires an argument of T[] and readOnly is a ReadonlyArray<T>.

Maybe it could treat Array<T> and ReadonlyArray<T> as two overloads.

There's no real need - since slice makes a copy, it's okay to accept ReadonlyArray (a strictly narrower type than Array) and always return the mutable Array.

@alxhub I updated the signature to accept ReadonlyArray<T> and return Array<T>

IgorMinar

Ok. Lgtm

Adds overloads to the `transform` methods of `SlicePipe`, to have better types than `any` for `value` and `any` as a return. With this commit, using `slice` in an `ngFor` still allow to type-check the content of the `ngFor` with `fullTemplateTypeCheck` enabled in Ivy: <div *ngFor="let user of users | slice:0:2">{{ user.typo }}</div> | `typo` does not exist on type `UserModel` whereas it is currently not catched (as the return of `slice` is `any`) neither in VE nor in Ivy. BREAKING CHANGE `SlicePipe` now only accepts an array of values, a string, null or undefined. This was already the case in practice, and it still throws at runtime if another type is given. But it is now a compilation error to try to call it with an unsupported type.

alxhub · 2019-05-17T19:01:14Z

Presubmit

Adds overloads to the `transform` methods of `SlicePipe`, to have better types than `any` for `value` and `any` as a return. With this commit, using `slice` in an `ngFor` still allow to type-check the content of the `ngFor` with `fullTemplateTypeCheck` enabled in Ivy: <div *ngFor="let user of users | slice:0:2">{{ user.typo }}</div> | `typo` does not exist on type `UserModel` whereas it is currently not catched (as the return of `slice` is `any`) neither in VE nor in Ivy. BREAKING CHANGE `SlicePipe` now only accepts an array of values, a string, null or undefined. This was already the case in practice, and it still throws at runtime if another type is given. But it is now a compilation error to try to call it with an unsupported type. PR Close #30156

Adds overloads to the `transform` methods of `SlicePipe`, to have better types than `any` for `value` and `any` as a return. With this commit, using `slice` in an `ngFor` still allow to type-check the content of the `ngFor` with `fullTemplateTypeCheck` enabled in Ivy: <div *ngFor="let user of users | slice:0:2">{{ user.typo }}</div> | `typo` does not exist on type `UserModel` whereas it is currently not catched (as the return of `slice` is `any`) neither in VE nor in Ivy. BREAKING CHANGE `SlicePipe` now only accepts an array of values, a string, null or undefined. This was already the case in practice, and it still throws at runtime if another type is given. But it is now a compilation error to try to call it with an unsupported type. PR Close angular#30156

angular-automatic-lock-bot · 2019-09-15T06:21:58Z

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.}

cexbrayat requested a review from a team as a code owner April 26, 2019 19:45

googlebot added the cla: yes label Apr 26, 2019

cexbrayat force-pushed the feat/type-slice branch 2 times, most recently from 1ff3059 to a52ffb9 Compare April 26, 2019 20:40

cexbrayat requested a review from a team as a code owner April 26, 2019 20:40

alfaproject reviewed Apr 26, 2019

View reviewed changes

cexbrayat force-pushed the feat/type-slice branch 2 times, most recently from 06dbaa7 to 07fc080 Compare April 26, 2019 21:09

trotyl reviewed Apr 27, 2019

View reviewed changes

cexbrayat force-pushed the feat/type-slice branch from 07fc080 to cfeb59d Compare April 27, 2019 07:43

IgorMinar added action: presubmit area: common labels Apr 29, 2019

ngbot bot added this to the needsTriage milestone Apr 29, 2019

kara added the action: review label May 7, 2019

kara approved these changes May 7, 2019

View reviewed changes

kara added action: merge target: patch action: review and removed action: presubmit action: review labels May 7, 2019

kara assigned IgorMinar May 7, 2019

alxhub requested changes May 7, 2019

View reviewed changes

IgorMinar approved these changes May 8, 2019

View reviewed changes

IgorMinar removed the action: review label May 8, 2019

alxhub added the action: cleanup label May 9, 2019

cexbrayat force-pushed the feat/type-slice branch from cfeb59d to 15032f9 Compare May 13, 2019 05:39

cexbrayat force-pushed the feat/type-slice branch from 15032f9 to 3fcdd14 Compare May 13, 2019 05:41

cexbrayat requested a review from alxhub May 13, 2019 06:04

alxhub approved these changes May 17, 2019

View reviewed changes

cexbrayat removed the action: cleanup label May 17, 2019

jasonaden closed this in 95830ee May 17, 2019

cexbrayat deleted the feat/type-slice branch June 27, 2019 11:35

iget-master mentioned this pull request Jul 10, 2019

CHANGELOG.md shouldn't include pre-release changes? #31491

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): stricter types for SlicePipe #30156

feat(common): stricter types for SlicePipe #30156

cexbrayat commented Apr 26, 2019 •

edited

Loading

alfaproject Apr 26, 2019

trotyl Apr 27, 2019

cexbrayat Apr 27, 2019

IgorMinar commented Apr 29, 2019

kara left a comment

kara commented May 7, 2019

kara commented May 7, 2019

alxhub May 7, 2019 •

edited

Loading

trotyl May 8, 2019

alxhub May 8, 2019

cexbrayat May 13, 2019

IgorMinar left a comment

alxhub commented May 17, 2019

angular-automatic-lock-bot bot commented Sep 15, 2019

feat(common): stricter types for SlicePipe #30156

feat(common): stricter types for SlicePipe #30156

Conversation

cexbrayat commented Apr 26, 2019 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

alfaproject Apr 26, 2019

Choose a reason for hiding this comment

trotyl Apr 27, 2019

Choose a reason for hiding this comment

cexbrayat Apr 27, 2019

Choose a reason for hiding this comment

IgorMinar commented Apr 29, 2019

kara left a comment

Choose a reason for hiding this comment

kara commented May 7, 2019

kara commented May 7, 2019

alxhub May 7, 2019 • edited Loading

Choose a reason for hiding this comment

trotyl May 8, 2019

Choose a reason for hiding this comment

alxhub May 8, 2019

Choose a reason for hiding this comment

cexbrayat May 13, 2019

Choose a reason for hiding this comment

IgorMinar left a comment

Choose a reason for hiding this comment

alxhub commented May 17, 2019

angular-automatic-lock-bot bot commented Sep 15, 2019

cexbrayat commented Apr 26, 2019 •

edited

Loading

alxhub May 7, 2019 •

edited

Loading