Don’t show auto-import suggestion if there’s a global by the same name#31065
Don’t show auto-import suggestion if there’s a global by the same name#31065andrewbranch wants to merge 2 commits intomicrosoft:masterfrom
Conversation
| verify.completions({ | ||
| marker: "", | ||
| // The real parseInt is in 'globalVars', the fake one exported from a.ts is missing | ||
| exact: ["globalThis", ...completion.globalsVars, "undefined", ...completion.statementKeywordsWithTypes], |
|
Another real world example that this PR would “break”: at my last job writing React, we had a commonly used component for text styling called |
|
@andrewbranch one way we could be smarter there is to not show types in places where only values are legal to write, or to offer import auto-fixes when a name resolves to a type but not a value but a value is auto-importable |
|
@RyanCavanaugh unfortunately in this case, |
|
My feeling is that this is now worse than the status quo, and I like #31893 better. |
Fixes #30713
Locals already shadow auto-import suggestions, but people were having issues with globals like
clearTimeoutbeing auto-imported from'timers', for example.I'm not 100% sure this is the right thing to do. Thinking about the
clearTimeoutexample:(Depending on the user’s tsconfig, they’ll likely have another ambient declaration in lib.dom.d.ts, but it becomes an overload with @types/node’s ambient declaration, so it’s not particularly relevant to the problem.)
Between these two declarations, I don't care about the one in
timersat all. I never ever want to importclearTimeoutfromtimers. Why?@types/nodeon accident or indirectly, and the import actually won’t work because I'm writing for the browser.On the other hand, I can envision some scenarios where I have different motivations and desired outcomes:
There's a DOM global
File, but as files are a pretty common thing to talk about in computing, it's also quite common to write a type or class calledFilein your own project. In this case, I probably want to auto-import my ownFileclass. The global gets ranked higher, but at least I can press the down arrow key and get the auto-import. If we merge this PR, nothing calledFilewill ever be auto-importable (in a program that has lib.dom.d.ts).Observations:
clearTimeoutdeclarations in@types/nodeare truly the same runtime function, but it seems highly likely since they’re both in@types/node.file.tsthan for a module we found innode_modulesliketimers.@types/nodeis special, because it’s the only exception I can think of to the rule: “If I have a package.json, I do not want to auto-import from any package not listed in it.”Possible solutions:
node_modulesclearTimeoutfromtimersgets hidden because it’s in@types/nodeand so is a global one.