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

Use node require for auto imports? #20292

Closed
mjbvz opened this issue Nov 27, 2017 · 8 comments · Fixed by #37027
Closed

Use node require for auto imports? #20292

mjbvz opened this issue Nov 27, 2017 · 8 comments · Fixed by #37027
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript VS Code Tracked There is a VS Code equivalent to this issue

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Nov 27, 2017

From microsoft/vscode#38773

Repo

Using the same project as #19629:

// The module 'vscode' contains the VS Code extensibility API
// Import the module and reference it with the alias vscode in your code below
// @ts-check

var vscode = require('vscode');

RelativePat|

Accept the completion for RelativePattern from vscode

Expected
Respects existing module style and adds require:

// The module 'vscode' contains the VS Code extensibility API
// Import the module and reference it with the alias vscode in your code below
// @ts-check

var vscode = require('vscode');
var { RelativePattern } = require('vscode');

RelativePattern

Actual
import inserted before everything else in the document:

import { RelativePattern } from 'vscode';

// The module 'vscode' contains the VS Code extensibility API
// Import the module and reference it with the alias vscode in your code below
// @ts-check

var vscode = require('vscode');

RelativePattern
@mjbvz mjbvz added the VS Code Tracked There is a VS Code equivalent to this issue label Nov 27, 2017
@mhegazy mhegazy added Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Nov 27, 2017
@oliversalzburg
Copy link

In our case, the feature is really more of an annoyance than anything else and I don't see how that could be solved.

Even if the issues pointed out @mjbvz are resolved, the automatic insertion does not respect code style guidelines to the extent that it would be acceptable.

  1. The quotation marks used are determined by the first string appearing in the current file. These may or may not be what is desired for imports. Although I admit that these should usually be consistent.

  2. As pointed out above, the import statement is placed at potentially invalid locations, an even worse case than pointed out above is when the file contains a shebang:

    import { read } from "fs";
    
    #!/usr/bin/env node

    This is now invalid and will not run.

  3. If you were to support require, would it use let, const or var? Regardless, it will be undesirable for someone.

  4. What determines if the line is terminated by a semicolon?

The biggest issue is that this happens out of sight and fails commits and/or builds due to code style violations. Handling this costs far more time than any auto-insertion of a statement would have saved.

IMHO, this should have been implemented through one of those little light bulb quick-actions, instead of auto-inserting the statement during auto-completion.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 28, 2017

As pointed out above, the import statement is placed at potentially invalid locations, an even worse case than pointed out above is when the file contains a shebang:

This is a bug that we are fixing. #20293 tracks that.

If you were to support require, would it use let, const or var? Regardless, it will be undesirable for someone.

I suppose const unless your --target is ES5/ES3.

What determines if the line is terminated by a semicolon?

tracked by #18780

The biggest issue is that this happens out of sight and fails commits and/or builds due to code style violations. Handling this costs far more time than any auto-insertion of a statement would have saved.

You can always disable the feature using "typescript.autoImportSuggestions.enabled": false if you so choose.

@oliversalzburg
Copy link

I suppose const unless your --target is ES5/ES3.

How is this determined in a pure JavaScript context?

You can always disable the feature using "typescript.autoImportSuggestions.enabled": false if you so choose.

I tried that. I put it into my workspace settings. It has no effect. After writing this, I realized that it is probably a user setting, which then worked. So, yay.

I'm actually not too concerned with the implementation details, because I doubt that this could be reasonably implemented without a clear definition of the user's desired code style anyway. I initially reported this as an issue because I was frustrated about this being enabled by default and having a negative impact on productivity (which is probably the opposite of what the feature wants to achieve). I feel this was enabled prematurely.

However, I should have realized earlier that the user settings control this behavior. Then I would have saved me said frustration.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 28, 2017

How is this determined in a pure JavaScript context?

Default is ES6, you can change it in jsconfig.json. but again this is my expectation to how the feature would work if implemented.

I'm actually not too concerned with the implementation details, because I doubt that this could be reasonably implemented without a clear definition of the user's desired code style anyway.

That is fair. we have been getting reports for adding configuration for style options like so. I think that is something we will be adding as we go. our preference however is to make it smart enough that it works out of the box, e.g. trying to infer the quote from the current file. or use a standard-compliant pattern that most ppl would find agreeable, e.g. using import syntax. I am sure there will always be more user cases to support, but we will keep chipping at these one feature at a time.

@danmarshall
Copy link

Hi @mhegazy - in my tsconfig.json, my compilerOptions module is set to "None". I am not using ES6 modules. VsCode is inserting import statements at the top of the file. Should it?

@lumaxis
Copy link

lumaxis commented Feb 23, 2018

I think it would be great to have a configuration option that generates require statements instead of imports and uses const/var depending on what the project is targeting 👍

@Undistraction
Copy link

Undistraction commented May 3, 2018

Just to add: There are definitely times when both require and import are required within the same project - code on node using require and front-end code using import, so a project-global 'importType' toggle won't be enough in these cases. There needs to be a more granular way of defining which should be used: possibly a regexp for files for each type.

Failing that, at least a way to disable auto import for some files. I'm woking with a Gatsby project at the moment and I'm having to remember to go and change every import for the node-related code.

@nykula
Copy link

nykula commented Nov 20, 2018

Could TypeScript auto-import with require() in JS code when tsconfig has "module": "commonjs" and "noEmit": true? I was looking for such a setting in VS Code but found only this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript VS Code Tracked There is a VS Code equivalent to this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants