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

feat(common): stricter types for SlicePipe #30156

Closed
wants to merge 1 commit into from

Conversation

@cexbrayat
Copy link
Contributor

commented Apr 26, 2019

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:

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

@cexbrayat cexbrayat requested a review from angular/fw-core as a code owner Apr 26, 2019

@googlebot googlebot added the cla: yes label Apr 26, 2019

@cexbrayat cexbrayat force-pushed the cexbrayat:feat/type-slice branch 2 times, most recently from 1ff3059 to a52ffb9 Apr 26, 2019

@cexbrayat cexbrayat requested a review from angular/fw-public-api as a code owner Apr 26, 2019

@@ -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(); });

This comment has been minimized.

Copy link
@alfaproject

alfaproject Apr 26, 2019

Contributor

as any would be less weird

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

@@ -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[];

This comment has been minimized.

Copy link
@trotyl

trotyl Apr 27, 2019

Contributor

I guess it should also accept ReadonlyArray<T>?

This comment has been minimized.

Copy link
@cexbrayat

cexbrayat Apr 27, 2019

Author Contributor

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

@cexbrayat cexbrayat force-pushed the cexbrayat:feat/type-slice branch from 07fc080 to cfeb59d Apr 27, 2019

@IgorMinar

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

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

kara approved these changes May 7, 2019

Copy link
Contributor

left a comment

LGTM. Running presubmit...

@kara

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@kara

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

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

@@ -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;

This comment has been minimized.

Copy link
@alxhub

alxhub May 7, 2019

Contributor

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

This comment has been minimized.

Copy link
@trotyl

trotyl May 8, 2019

Contributor

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

This comment has been minimized.

Copy link
@alxhub

alxhub May 8, 2019

Contributor

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.

This comment has been minimized.

Copy link
@cexbrayat

cexbrayat May 13, 2019

Author Contributor

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

@IgorMinar
Copy link
Member

left a comment

Ok. Lgtm

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

feat(common): stricter types for SlicePipe
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.

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

@cexbrayat cexbrayat requested a review from alxhub May 13, 2019

@alxhub

alxhub approved these changes May 17, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@jasonaden jasonaden closed this in 95830ee May 17, 2019

jasonaden added a commit that referenced this pull request May 17, 2019

feat(common): stricter types for SlicePipe (#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 #30156

BioPhoton added a commit to BioPhoton/angular that referenced this pull request May 21, 2019

feat(common): stricter types for SlicePipe (angular#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.