Conversation
877401f to
b490813
Compare
b490813 to
f0ac1ff
Compare
f0ac1ff to
210e4c2
Compare
|
@ranma42 I've fixed up the language service part, should be good to go now. |
kyliau
left a comment
There was a problem hiding this comment.
LGTM for language service
petebacondarwin
left a comment
There was a problem hiding this comment.
Apart from the additional suggested overloads, which IMO are optional for this PR, LGTM
Reviewed-for: fw-i18n
There was a problem hiding this comment.
Is it worth adding yet another overload of
transform(value: null|undefined): null;There was a problem hiding this comment.
And similarly elsewhere.
(Disclaimer: I have not checked whether this would break the type system...)
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Do we not prefer use of isNaN?
| return !(value == null || value === '' || value !== value); | |
| return !(value == null || value === '' || isNaN(value)); |
There was a problem hiding this comment.
TypeScript rejects it because value is typed as number|string at that point.
There was a problem hiding this comment.
We could go crazy here and do something like:
transform(value: number, ...): string;
transform(value: null | undefined, ...): '';
transform(value: number | null | undefined, ...): string;There was a problem hiding this comment.
sure! :) should I go forward with that?
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Reviewed-for: public-api
Nice work @ranma42 - thanks for your patience.
A typo in commit 1:
instead
reflctsreflects the behaviour onnullandundefined.
And a few nits and suggestions.
…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.
1d61f61 to
967dd3e
Compare
JoostK
left a comment
There was a problem hiding this comment.
LGTM, thanks for putting this together!
`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
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
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
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
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
|
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! |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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?
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