fix(compiler-cli): report more accurate diagnostic for invalid import #60455
Conversation
| @@ -132,7 +138,21 @@ export function validateAndFlattenComponentImports( | |||
| ), | |||
| ); | |||
| } else { | |||
| diagnostics.push(createValueHasWrongTypeError(expr, imports, errorMessage).toDiagnostic()); | |||
| const isStaticArray = | |||
There was a problem hiding this comment.
Optional nit: I wonder if it makes sense to just limit to isArrayExpression(expr) && expr.elements.every(x => ts.isIdentifier(x)) && !imports.some(Array.isArray). I don't know for what else this logic might be true.
There was a problem hiding this comment.
I was trying to account for this case which I think is allowed:
const IMPORTS = [...];
imports: [Foo, IMPORTS];
There was a problem hiding this comment.
ah I see. Aren't you excluding that cass below with the some isArray check?
There was a problem hiding this comment.
Ah sorry, I thought your snippet excluded the isArray check but it's actually the length check. I put it in there as a quicker way to exit in case there's a spread.
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken |
packages/compiler-cli/src/ngtsc/annotations/component/src/util.ts
Outdated
Show resolved
Hide resolved
Currently when an incorrect value is in the `imports` array, we highlight the entire array which can be very noisy for large arrays. This comes up semi-regularly (at least for me) when an import is missing. These changes add some logic that reports a more accurate diagnostic location for the most common case where the `imports` array is static. Non-static arrays will fall back to the current behavior.
eb279d3 to
ad0b8ab
Compare
| let diagnosticValue: ResolvedValue; | ||
|
|
||
| if (ref instanceof DynamicValue) { | ||
| diagnosticNode = ref.node; |
There was a problem hiding this comment.
Without getOriginNodeForDiagnostics I think the following may perhaps become unclear:
const IMPORTS = [HelloDir, InvalidReference];
@Component({ imports: IMPORTS })
export class MyCmp {}Assuming InvalidReference is a DynamicValue (e.g. because an import is missing/invalid), the diagnostic would be reported within the IMPORTS initializer, separate from the component declaration. This becomes more unclear when IMPORTS is declared in a different source file.
I suspect that using getOriginNodeForDiagnostics would mostly behave the same as the explicit array literal check covers (making it redundant), except for cases where the imports array literal contains a spread element (so possibly still meaningful).
|
This PR was merged into the repository by commit 29eded6. The changes were merged into the following branches: main, 19.2.x |
…#60455) Currently when an incorrect value is in the `imports` array, we highlight the entire array which can be very noisy for large arrays. This comes up semi-regularly (at least for me) when an import is missing. These changes add some logic that reports a more accurate diagnostic location for the most common case where the `imports` array is static. Non-static arrays will fall back to the current behavior. PR Close #60455
|
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. |
Currently when an incorrect value is in the
importsarray, we highlight the entire array which can be very noisy for large arrays. This comes up semi-regularly (at least for me) when an import is missing.These changes add some logic that reports a more accurate diagnostic location for the most common case where the
importsarray is static. Non-static arrays will fall back to the current behavior.Example of what I'm referring to:
