Skip to content

Don’t show auto-import suggestion if there’s a global by the same name #31065

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

andrewbranch
Copy link
Member

Fixes #30713

Locals already shadow auto-import suggestions, but people were having issues with globals like clearTimeout being auto-imported from 'timers', for example.

I'm not 100% sure this is the right thing to do. Thinking about the clearTimeout example:

// node_modules/@types/node/globals.d.ts
declare function clearTimeout(timeoutId: NodeJS.Timeout): void;

// node_modules/@types/node/timers.d.ts
declare module 'timers' {
  function clearTimeout(timeoutId: NodeJS.Timeout): void;
}

(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 timers at all. I never ever want to import clearTimeout from timers. Why?

  1. If I'm writing for node, I don't want the import because it's equivalent to the global, and it's unconventional to import it. I probably don't even know that a module called “timers” exists.
  2. If I'm writing for the browser, I probably have @types/node on 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:

// file.ts
export class File {}

// index.ts
const file = new Fi/**/

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 called File in your own project. In this case, I probably want to auto-import my own File class. 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 called File will ever be auto-importable (in a program that has lib.dom.d.ts).

Observations:

  • We can't actually know that the two clearTimeout declarations in @types/node are truly the same runtime function, but it seems highly likely since they’re both in @types/node.
  • If a module export shares the same name with a global, the chances that you want to import that thing instead of use the global are probably much higher for a local module like file.ts than for a module we found in node_modules like timers.
  • @types/node is 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:

  • Retain same-name auto-import suggestions for local modules, suppress them for node_modules
  • Suppress auto-import suggestions that have the same name as an ambient declaration from the same package; i.e. clearTimeout from timers gets hidden because it’s in @types/node and so is a global one.
  • Just go with this PR and see if compelling arguments for doing it another way arise in practice.

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],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandersn What's going on with globalThis here?

@andrewbranch
Copy link
Member Author

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 <Text />. Text is also the name of an interface in lib.dom.d.ts. I auto-imported Text the component all the time and literally never wanted to reference Text the DOM interface, and it was a very minor nuisance to have to down-arrow to get the auto-import. With this PR, auto importing Text will no longer be possible 😕

@RyanCavanaugh
Copy link
Member

@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

@andrewbranch
Copy link
Member Author

@RyanCavanaugh unfortunately in this case, Text is both a type and a value (it's a constructor). I meant to try to get more info in #30713 (I typed up a whole comment with screenshots) but it's gone somehow 🤔

@andrewbranch
Copy link
Member Author

My feeling is that this is now worse than the status quo, and I like #31893 better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clearTimeout is imported from timers
3 participants