Repository navigation
Conversation
There was a problem hiding this comment.
Using @ts-expect-error instead of a type assertion improves the code navigation.
There was a problem hiding this comment.
Few month have passed, I actually want to push for using the Writable type, cf #49754
fbc23b6 to
6f4f4ca
Compare
There was a problem hiding this comment.
Maybe it could removed in separate PR with a move of DefaultIterableDiffer to the private space.
pkozlowski-opensource
left a comment
pkozlowski-opensource
left a comment
There was a problem hiding this comment.
@JeanMeche thnx for the PR and sorry that it took so much time to process.
I've went through 1/2 of it and I would love to discus the usage of ! for @Input fields.
Generally my thinking is that inputs (and other non-query fields) would be better off with a default value or adding undefined to the type.
My reasoning is that queries are assigned by the framework and the author of the component has full control over assign timing / usage. With @Inputs (and other fields) there are no guarantees.
It doesn't matter the whole lot but people often look at Angular tests for examples / "inspiration" so we should decide on the pattern here.
WDYT?
There was a problem hiding this comment.
boolean|undefined? or inp = false?
There was a problem hiding this comment.
Generally my thinking is that inputs (and other non-query fields) would be better off with a default value or adding undefined to the type.
My reasoning is that queries are assigned by the framework and the author of the component has full control over assign timing / usage. With @Inputs (and other fields) there are no guarantees.
It doesn't matter the whole lot in this particular case but people often look at Angular tests for examples / "inspiration" so we should decide on the pattern here.
There was a problem hiding this comment.
You last argument nails it for me. Every bit a code that is accessible to the devs should be of the highest standards !
I will update the PR.
pkozlowski-opensource
added
action: cleanup
cc9d18f to
a18c887
Compare
pkozlowski-opensource
left a comment
pkozlowski-opensource
left a comment
There was a problem hiding this comment.
LGTM
Reviewed-for: fw-core
Reviewed-for: fw-security
pkozlowski-opensource
added
action: review
dylhunn
left a comment
dylhunn
left a comment
There was a problem hiding this comment.
Needs one more fw-security approval. Perhaps @josephperrott can provide the stamp? It's basically a no-op in that regard.
alxhub
removed
the
action: review
pkozlowski-opensource
added
the
action: merge
|
@JeanMeche it looks like this PR will need a rebase after the recent merges of your other PR :-) |
pkozlowski-opensource
added
the
action: cleanup
This commit removes the remaining TODO(issue/24571) in core code base.
a18c887 to
9a770e6
Compare
|
@pkozlowski-opensource There you go ! You can merge it now =) |
pkozlowski-opensource
removed
the
action: cleanup
|
Thnx @JeanMeche ! |
pkozlowski-opensource
added
action: presubmit
|
This PR was merged into the repository by commit 8d42747. |
|
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. |
This commit removes the remaining TODO(issue/24571) in core code base. PR Close angular#49221
6 participants
This commit removes the remaining TODO(issue/24571) in core code base.
In the core code base most TODOs were on spec files where it is acceptable to have non-null assertions.
See also the PRs about #24571 :
*#48241