Skip to content
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

clearTimeout is imported from timers #30713

Closed
csicky opened this issue Apr 2, 2019 · 8 comments
Closed

clearTimeout is imported from timers #30713

csicky opened this issue Apr 2, 2019 · 8 comments
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue Fixed A PR has been merged for this issue

Comments

@csicky
Copy link

csicky commented Apr 2, 2019

  • VSCode Version: 1.32.3
  • OS Version: Windows 10

Steps to Reproduce:

  1. Write clearTimeout
  2. You get import { clearTimeout } from 'timers' imported without asking and error in the browser.

Reported here already:
microsoft/vscode#57822
microsoft/vscode#40001
microsoft/vscode#38911

Is it impossible for VS Code to know that I write code for a browser where these are available already, and should not import them?

Does this issue occur when all extensions are disabled?: Yes/No - no idea

@mjbvz mjbvz transferred this issue from microsoft/vscode Apr 2, 2019
@DanielRosenwasser
Copy link
Member

I think this is a duplicate of #15024.

@csicky
Copy link
Author

csicky commented Apr 5, 2019

I don't think it's a duplicate of #15024 because that issue says to favor local variables while this issue is about why existing global properties of the window object are being shadowed with something auto imported. This was and will be reported probably in the future because it doesn't seem like there is any interest in fixing this.

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Apr 9, 2019
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Apr 9, 2019

I see both in my completion list, which is probably even worse (since it's hard to tell them apart). If something in the lexical scope exists, we shouldn't offer it at all as an auto-import

@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.5.0 milestone Apr 9, 2019
@zpdDG4gta8XKpMCd
Copy link

this is how i do it:

window.setTimeout(...) // <-- right signature is picked

it's back to the question about annoying ambients #14306

palantir/tslint#922

@andrewbranch
Copy link
Member

@csicky I'm trying to track down how and when the worst-case scenario is happening. Like Ryan, I see both completions, and the global one is always the top one and always selected, so for me, clearTimeout never gets accidentally imported because I have to press the down arrow key to select the auto-import. Could you help me try to reproduce your exact problem?

  1. You're writing JavaScript, not TypeScript, correct?
  2. Is there a tsconfig.json file anywhere in your project?
  3. What do you see when you start typing clearTimeout? Here’s what I see:

Screen Shot 2019-04-30 at 6 32 36 AM

4. In VS Code, if you open the command palette and select “Preferences: Open Settings (JSON),” are there any options you have set that begin with `editor.suggest`?

@csicky
Copy link
Author

csicky commented Apr 30, 2019

Yes I have this one: "editor.suggest.localityBonus": true,

Right now I have re-enabled the import setting and like you said, the globals are now on top of the list. The auto import was on top at the time I opened this issue, so that is fixed.

Still, I think that there shouldn't be so confusing imports shadowing globals like in the following image:
image

I am not sure which setting is responsible for the auto import in a .vue file component in a project without TypeScript. For this test I enabled all 3 of them (temporarily).

  "typescript.suggest.autoImports": true,
  "javascript.suggest.autoImports": true,
  "vetur.completion.autoImport": true,

@andrewbranch
Copy link
Member

andrewbranch commented May 23, 2019

For public visibility: I do have a PR up at #31065, but I don't like it very much. Given this:

the globals are now on top of the list. The auto import was on top at the time I opened this issue, so that is fixed.

I've changed the milestone to 3.6 and plan to revisit this and see if we can come up with some better heuristics after 3.5 is released.

@andrewbranch
Copy link
Member

Woops, this should have been closed with #31893. The "fixed" label kept it from showing up in my usual query.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants