Sitelet https://github.com/angular/angular/pull/37447
Skip to content

Improve typing of common pipes - #37447

Closed
ranma42 wants to merge 7 commits into
angular:masterfrom
ranma42:fix/common-pipes-typing
Closed

ranma42 wants to merge 7 commits into
angular:masterfrom
ranma42:fix/common-pipes-typing

Conversation

@ranma42

@ranma42 ranma42 commented Jun 4, 2020

Copy link
Copy Markdown
Contributor

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: #36259
Several pipes have incomplete or incorrect typing.

What is the new behavior?

The common pipes should now all have a stricter typing.

Does this PR introduce a breaking change?

  • Yes
  • No

It depends on what is considered a breaking change: some code that would compile will now trigger a compilation error. On the other hand, the code that is made invalid by this change, such as passing an object to a numeric pipe, was incorrect in the first place (it caused errors at runtime instead of at compile time).

Fixing the fallout should be trivial.

Other information

Originally posted as #36277

@pullapprove
pullapprove Bot requested review from kara, kyliau and mhevery June 4, 2020 23:56
@kyliau
kyliau requested review from petebacondarwin and removed request for kara June 4, 2020 23:58
@pullapprove
pullapprove Bot requested a review from kara June 4, 2020 23:58
@kyliau

kyliau commented Jun 4, 2020

Copy link
Copy Markdown
Contributor

Thank you for reopening the PR @ranma42!

For context, this is a continuation of #36277. It was accidentally closed by me due to my silly mistake in git push!

@kyliau
kyliau force-pushed the fix/common-pipes-typing branch from 877401f to b490813 Compare June 5, 2020 03:15
@kyliau
kyliau requested a review from ayazhafiz June 5, 2020 03:17
@kyliau
kyliau force-pushed the fix/common-pipes-typing branch from b490813 to f0ac1ff Compare June 5, 2020 03:20
Comment thread packages/common/src/pipes/number_pipe.ts Outdated
Comment thread packages/common/src/pipes/slice_pipe.ts Outdated
Comment thread packages/common/test/pipes/keyvalue_pipe_spec.ts Outdated
Comment thread packages/language-service/src/typescript_symbols.ts Outdated
Comment thread packages/language-service/src/typescript_symbols.ts Outdated
@kyliau
kyliau force-pushed the fix/common-pipes-typing branch from f0ac1ff to 210e4c2 Compare June 5, 2020 04:02
@kyliau kyliau added area: common Issues related to APIs in the @angular/common package target: patch This PR is targeted for the next patch release labels Jun 5, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Jun 5, 2020
@kyliau

kyliau commented Jun 5, 2020

Copy link
Copy Markdown
Contributor

@ranma42 I've fixed up the language service part, should be good to go now.
@petebacondarwin Could you please take another look? Sorry I messed up the earlier PR and accidentally closed it.

@kyliau kyliau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for language service

@petebacondarwin petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the additional suggested overloads, which IMO are optional for this PR, LGTM

Reviewed-for: fw-i18n

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth adding yet another overload of

transform(value: null|undefined): null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And similarly elsewhere.

(Disclaimer: I have not checked whether this would break the type system...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried locally and it looks like it should be fine, apart from a couple of language-service tests that would need to be updated (they check that there is exactly one type for a pipe, while this change would make the resolver find two types).
Should I add it to this branch or is it better to do it in another PR?
In the first case, should I squash the changes or add them on top? (what is best for reviews/the convention for this repo?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth adding yet another overload of

transform(value: null|undefined): null;

I had omitted them assuming that it is a niche use case, but it certainly makes typing more accurate and I guess it might help spotting issues in some buggy code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for this PR from my point of view. I just mention it because, this PR involves a public API change, and a second PR would also do so, which is extra work checking the API each time.

I would rather get @IgorMinar another public API approver to comment before you make any further changes.

@kyliau
kyliau requested review from kara and removed request for IgorMinar, alxhub, kara and pkozlowski-opensource June 8, 2020 18:47
@pullapprove
pullapprove Bot requested a review from alxhub June 8, 2020 18:57

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not prefer use of isNaN?

Suggested change
return !(value == null || value === '' || value !== value);
return !(value == null || value === '' || isNaN(value));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript rejects it because value is typed as number|string at that point.

Comment thread goldens/public-api/common/common.d.ts Outdated

@petebacondarwin petebacondarwin Sep 21, 2020 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could go crazy here and do something like:

    transform(value: number, ...): string;
    transform(value: null | undefined, ...): '';
    transform(value: number | null | undefined, ...): string;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure! :) should I go forward with that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a way, it looks like a good idea, as it provides a type-level "documentation" of the behavior in these special cases.
OTOH, it looks like a strange commitment from the perspective of the API

@petebacondarwin petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: public-api

Nice work @ranma42 - thanks for your patience.

A typo in commit 1:

instead reflctsreflects the behaviour on null and undefined.

And a few nits and suggestions.

@petebacondarwin petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker state: blocked labels Sep 21, 2020
Comment thread packages/common/src/pipes/keyvalue_pipe.ts Outdated
Comment thread packages/common/src/pipes/keyvalue_pipe.ts Outdated
Comment thread packages/common/src/pipes/number_pipe.ts Outdated
Comment thread packages/common/test/pipes/number_pipe_spec.ts Outdated
…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.
`AsyncPipe.transform` will never return `undefined`, even when passed
`undefined` in input, in contrast with what was declared in the
overloads.

Additionally the "actual" method signature can be updated to match the
most generic case, since the implementation does not rely on wrappers
anymore.

BREAKING CHANGE:
The async pipe no longer claims to return `undefined` for an input that
was typed as `undefined`. Note that the code actually returned `null` on
`undefined` inputs. In the unlikely case you were relying on this,
please fix the typing of the consumers of the pipe output.
Make typing of DatePipe stricter to catch some misuses (such as passing
an Observable or an array) at compile time.

BREAKING CHANGE:
The signature of the `date` pipe now explicitly states which types are
accepted. This should only cause issues in corner cases, as any other
values would result in runtime exceptions.
Make typing of number pipes stricter to catch some misuses (such as
passing an Observable or an array) at compile time.

BREAKING CHANGE:
The signatures of the number pipes now explicitly state which types are
accepted. This should only cause issues in corner cases, as any other
values would result in runtime exceptions.
I18nPluralPipe can actually accept `null` and `undefined` (which are
convenient for composing it with the async pipe), but it is currently
typed to only accept `number`.
As shown in the tests, `KeyValuePipe.transform` can accept
`undefined`, in which case it always returns `null`.

Additionally, the typing for `string` keys can be made generic, so the
comparison function is only required to accept the relevant cases.

Finally, the typing for `number` records now shows that the comparison
function and the result entries will actually receive the string version
of the numeric keys, just as shown in the tests.

BREAKING CHANGE:
The typing of the `keyvalue` pipe has been fixed to report that for
input objects that have `number` keys, the result will contain the
string representation of the keys. This was already the case and the
code has simply been updated to reflect this. Please update the
consumers of the pipe output if they were relying on the incorrect
types. Note that this does not affect use cases where the input values
are `Map`s, so if you need to preserve `number`s, this is an effective
way.
Even in the overloads, state that it can accept `null` and
`undefined`, in order to ensure easy composition with `async`.

Additionally, change the implementation to return `null` on an
`undefined` input, for consistency with other pipes.

BREAKING CHANGE:
The `slice` pipe now returns `null` for the `undefined` input value,
which is consistent with the behavior of most pipes. If you rely on
`undefined` being the result in that case, you now need to check for it
explicitly.
@ranma42
ranma42 force-pushed the fix/common-pipes-typing branch from 1d61f61 to 967dd3e Compare September 23, 2020 09:30

@JoostK JoostK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for putting this together!

@AndrewKushnir

AndrewKushnir commented Sep 23, 2020 •

Copy link
Copy Markdown
Contributor

Presubmit + Global Presubmit.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit labels Sep 23, 2020
@alxhub alxhub closed this in c7d5555 Sep 28, 2020
alxhub pushed a commit that referenced this pull request Sep 28, 2020
`AsyncPipe.transform` will never return `undefined`, even when passed
`undefined` in input, in contrast with what was declared in the
overloads.

Additionally the "actual" method signature can be updated to match the
most generic case, since the implementation does not rely on wrappers
anymore.

BREAKING CHANGE:
The async pipe no longer claims to return `undefined` for an input that
was typed as `undefined`. Note that the code actually returned `null` on
`undefined` inputs. In the unlikely case you were relying on this,
please fix the typing of the consumers of the pipe output.

PR Close #37447
alxhub pushed a commit that referenced this pull request Sep 28, 2020
Make typing of DatePipe stricter to catch some misuses (such as passing
an Observable or an array) at compile time.

BREAKING CHANGE:
The signature of the `date` pipe now explicitly states which types are
accepted. This should only cause issues in corner cases, as any other
values would result in runtime exceptions.

PR Close #37447
alxhub pushed a commit that referenced this pull request Sep 28, 2020
Make typing of number pipes stricter to catch some misuses (such as
passing an Observable or an array) at compile time.

BREAKING CHANGE:
The signatures of the number pipes now explicitly state which types are
accepted. This should only cause issues in corner cases, as any other
values would result in runtime exceptions.

PR Close #37447
alxhub pushed a commit that referenced this pull request Sep 28, 2020
I18nPluralPipe can actually accept `null` and `undefined` (which are
convenient for composing it with the async pipe), but it is currently
typed to only accept `number`.

PR Close #37447
alxhub pushed a commit that referenced this pull request Sep 28, 2020
As shown in the tests, `KeyValuePipe.transform` can accept
`undefined`, in which case it always returns `null`.

Additionally, the typing for `string` keys can be made generic, so the
comparison function is only required to accept the relevant cases.

Finally, the typing for `number` records now shows that the comparison
function and the result entries will actually receive the string version
of the numeric keys, just as shown in the tests.

BREAKING CHANGE:
The typing of the `keyvalue` pipe has been fixed to report that for
input objects that have `number` keys, the result will contain the
string representation of the keys. This was already the case and the
code has simply been updated to reflect this. Please update the
consumers of the pipe output if they were relying on the incorrect
types. Note that this does not affect use cases where the input values
are `Map`s, so if you need to preserve `number`s, this is an effective
way.

PR Close #37447
@AndrewKushnir

Copy link
Copy Markdown
Contributor

Hi @ranma42, just want to let you know that this PR was merged into master branch and the change will be included into upcoming v11 release. Thank you for contributing to Angular!

@angular-automatic-lock-bot

Copy link
Copy Markdown

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.

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

Labels

action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package breaking changes cla: yes cross-cutting: types target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add Promise<T> | Observable<T> async pipe overload