refactor: improve string export name completions #58818
Conversation
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
typescript-bot
added
the
For Uncommitted Bug
src/services/completions.ts
Outdated
| if (needsConvertPropertyAccess && contextToken?.kind === SyntaxKind.DotToken && contextToken.parent.kind === SyntaxKind.QualifiedName) { | ||
| replacementSpan = createTextSpanFromNode(contextToken, sourceFile); | ||
| insertText = `[${quotePropertyName(sourceFile, preferences, name)}]`; | ||
| } |
There was a problem hiding this comment.
Is it worth doing this if it's guaranteed to be wrong, compared to just not offering this?
There was a problem hiding this comment.
Yes, and I hope QualifiedName can contain ["stringLiteral"] in the future, since now a type name may not be an identifier.
There was a problem hiding this comment.
What does the error message say when you land in this position? IMO it’s very annoying and confusing for completions to be invalid. It might be ok in this edge case if the diagnostic explains what’s going on and how to fix it, but if it’s just Module 'foo' has no export "non identifier", then I think we need to do better somehow, and I’d lean toward not offering the completion in the meantime.
There was a problem hiding this comment.
Code:
type z = x.Before this PR:
type z = x.a b
// Namespace '...' has no exported member 'a'.
// ';' expected.
// Cannot find name 'b'.After this PR:
type z = x['a b']
// Cannot use namespace 'x' as a type.I think the error message becomes better
There was a problem hiding this comment.
But is that better than not offering the broken completion at all? I’m not sure.
|
ping @andrewbranch please take a look again thanks! |
|
My latest feedback stands—I don’t think we should offer invalid completions. |
I still think the completion I gave was better, but I removed that to satisfy the review requirement. Can you review this now, thanks! |
|
Thanks @Jack-Works! |
continue of #49297 and #58640