Conversation
There was a problem hiding this comment.
Disclaimer: I'm not entirely sure if this is the best way to find which array the symbols was imported by.
There was a problem hiding this comment.
I think this will catch a few things you might not expect too, such as inline nested arrays imports: [X, Y, [Target, Z]].
There was a problem hiding this comment.
I think that reporting imports: [X, Y, [Target, Z]] is the correct behavior since it's defined inline. What I want to avoid is arrays shared between components.
crisbeto
added
action: review
There was a problem hiding this comment.
I would consider it meaningful the extend this comment with the why. AFAIK this is just a heuristic to determine if the array is possibly shared, and therefore shouldn't be considered owned by the template.
Note that this heuristic has a limitation, where multiple components in the same file that reuse a single constant array may report false positives. These could potentially be avoided by also keeping track of which identifiers are used, and then have a post-file visit operation to determine after the source file has fully been scanned which of the unused identifiers are unused throughout the source file. I'm not sure if that is worth it, but perhaps good to note in the comment here the possibility for false positives.
There was a problem hiding this comment.
Do we want to have this nuance? Clear and simple rules are preferable to heuristics in a lot of cases.
There was a problem hiding this comment.
I'd be fine keeping this simple, but I would call this out in a comment to at least capture that it's been considered and decided against for simplicity.
There was a problem hiding this comment.
Or do you mean the nuance of differentiating between exported arrays at all?
…mported array Some apps follow a pattern where they have an array of common declarations which is imported in most standalone components, but only some of the declarations are used. Such cases will currently raise the unused imports diagnostic but can be hard to fix, because it would require either removing declarations from the common array which can break other components, or copying only the necessary declarations from the array. Since neither of these solutions is great, this commit tweaks the logic for the diagnostic so that unused imports coming from _exported_ arrays are not reported (either from the same file or another one).
47dc321 to
ff1c013
Compare
angular-robot
Bot
removed
the
area: compiler
crisbeto
left a comment
crisbeto
left a comment
There was a problem hiding this comment.
I've addressed the feedback.
There was a problem hiding this comment.
I think that reporting imports: [X, Y, [Target, Z]] is the correct behavior since it's defined inline. What I want to avoid is arrays shared between components.
crisbeto
added
action: merge
|
This PR was merged into the repository by commit 33fe252. The changes were merged into the following branches: main |
|
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. |
3 participants
Some apps follow a pattern where they have an array of common declarations which is imported in most standalone components, but only some of the declarations are used. Such cases will currently raise the unused imports diagnostic but can be hard to fix, because it would require either removing declarations from the common array which can break other components, or copying only the necessary declarations from the array. Since neither of these solutions is great, this commit tweaks the logic for the diagnostic so that unused imports coming from exported arrays are not reported (either from the same file or another one).