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

autoimport should care about semicolons #19882

Closed
mjbvz opened this issue Nov 9, 2017 · 46 comments · Fixed by #31801
Closed

autoimport should care about semicolons #19882

mjbvz opened this issue Nov 9, 2017 · 46 comments · Fixed by #31801
Assignees
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue 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 9, 2017

From @phra on November 9, 2017 20:11

i usually write my ts/js code without semicolons and there is now way to configure vscode to avoid adding semicolons to auto imports. it would be nice an automatic parsing of the tslint config or at least a configurable option in the settings.

  • VSCode Version: 1.18
  • OS Version: Linux

Steps to Reproduce:

  1. use new autoimport feature
  2. imports are always with semicolons

Copied from original issue: microsoft/vscode#37991

@mjbvz mjbvz self-assigned this Nov 9, 2017
@mjbvz mjbvz added VS Code Tracked There is a VS Code equivalent to this issue and removed javascript labels Nov 9, 2017
@mjbvz mjbvz removed their assignment Nov 9, 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 9, 2017
@ghost
Copy link

ghost commented Nov 9, 2017

It looks like FormatCodeSettings only controls whitespace, so this would have to be an emitter option?

@mhegazy
Copy link
Contributor

mhegazy commented Nov 9, 2017

We could add a new set of CodeFixSettings that include semicolon and quotes as well as any other things that would be needed by code fixes..

alternatively, we can teach the formatter about semi colons.

@ghost
Copy link

ghost commented Nov 9, 2017

It's probably better to keep all of those settings in one place. So your second suggestion sounds best.

@Lxxyx
Copy link

Lxxyx commented Nov 13, 2017

I like ts importer. You can configure semicon, space by youself.

https://github.com/pmneo/ts-importer

image

@phra
Copy link

phra commented Nov 13, 2017

yep, i think that updating the formatter can be the best option too.

@timc13
Copy link

timc13 commented Jan 19, 2018

just to add my 2 cents, a few lint rules can probably fixed at the same time. Here are the rules that are breaking for me when I auto fix an import:

@ksmutny
Copy link

ksmutny commented Jan 19, 2018

Why doesn't the TS AutoImport just respect TS Lint settings?

@phra
Copy link

phra commented Mar 15, 2018

any news?

@mhegazy
Copy link
Contributor

mhegazy commented Mar 29, 2018

With #22236 added, we can add a configuration option for this feature. we would be happy to take a PR for this.

@mhegazy mhegazy added the Help Wanted You can do this label Mar 29, 2018
@mhegazy mhegazy added this to the Community milestone Mar 29, 2018
@chenasraf
Copy link

chenasraf commented May 28, 2018

I'd be happy to work on this, can anyone point me to the general direction of where I should look?
Where is the auto import logic stored?

@RyanCavanaugh RyanCavanaugh removed the In Discussion Not yet reached consensus label Mar 14, 2019
@khaosdoctor
Copy link

I'm trying to find this piece of code and set it up in a PR, however it is a bit foggy for my about in what project is this problem related to. It'd be nice to have a VSCode setting about this, however this seems to be in the TS server.

@mjomble
Copy link

mjomble commented Mar 19, 2019

@khaosdoctor we should probably first add a new settings for it in the TS server (this repo) and then make a change in the VS Code repo that would use this setting.

More thoughts and suggestions in this comment

@khaosdoctor
Copy link

Yeah, I just talked to some people here and they have explained me this. We need to add the logic to read this setting in the TS server and another PR to VSCode to create this setting altogether.

@rdewolff
Copy link

I keep cleaning our projects files' import as VSCode is adding semicolon automatically and we always forget some.

Any plan to do this?

@mlshv
Copy link

mlshv commented May 22, 2019

I use VSCode for more than 2 years (and really enjoy it, btw). One of the first bugs I've noticed was this: when you use autoimport it always adds a semicolon in the end of the line. Even if you don't want it and have all kinds of settings for it. I thought "Ok, it's a pretty obvious and simple thing, they gotta fix it really soon". I was waiting for ages. And today I finally decided to find out what's going on.

Is it so complicated? Does somebody work on this issue? Is there any way to help?

@khaosdoctor
Copy link

@mlshv It's pretty tricky since you have to add two settings, one in the TS Language Server, to allow the use of semicolons, and another one in the VSCode to read the previous one.

We discuss this in some previous comments

@andrewbranch
Copy link
Member

There's a milestone and an assignee now → It ✨ should ✨ get done for 3.6 🙂

@cancerberoSgx
Copy link

as @mjomble said, Printer doesn't seem to be respecting omitTrailingSemicolon, np matter its value it always print trailing semicolon, would be awesome if this is also fixed, thanks

@rob2d
Copy link

rob2d commented Aug 4, 2019

I noticed that there was an algorithm for detecting semi colons, but in this fix (that has been closed), does it only apply this to import checks? the reason I ask is I have seen it very common in certain codebases (especially React) not to include semi colons for import/export, but to do so for all other code. If all that was considered was the entire file having at least 3/4 semi colons on each statement, it would not detect this sort of scenario properly or intelligently.

@andrewbranch
Copy link
Member

@rob2d no, the fix considers any statements that normally have semicolons but don’t require them.

If all that was considered was the entire file having at least 3/4 semi colons on each statement

That said, it starts looking from the top of the file and quits once it sees a handful of examples to work with, so if you had a bunch of semicolon-less imports at the top of your file, it should infer that you don’t want semicolons. So in that case, auto-imports will do what you want, and other code insertions won’t.

I have seen it very common in certain codebases (especially React) not to include semi colons for import/export, but to do so for all other code

Before joining TypeScript, I worked heavily in React for several years, and I’ve never heard of such a thing. Can you send some examples?

@rob2d
Copy link

rob2d commented Aug 5, 2019

@andrewbranch thanks for explanation on first part 🙃

I'm not doubting that you have experience working in React, but some of the most popular OSS libraries for their domain follow this convention so that is a little bit weird... Perhaps you had seen it and not noticed? I've been using React since v0.3.x and remember literally > half of the libraries that I was importing at one point used this convention, and many very popular and upcoming repos still follow it to some extent... though I can recognize it's gaining popularity to use semis since VSCode is now becoming a very good ES7 IDE -- thanks partly to TS.

There is also an option for this in ESLint and Prettier -- ironically what causes many to be give up is strong adoption of VSCode which thanks to TS has defaults which are not quite as accommodating for the entire community (though I do have to say I am very impressed with it's rapid improvement/progress).

Just to emphasize I'm not simply stubborn or something: the main reason I do not use semis on imports personally is because it is a bit noisy to look at in the top of my files -- have a bit of OCD about clutter with imports and can justify mentally separating it as the only statically linked part of an ES7 app.

Obviously though, in codebases I am touching that do not share this view, "when in Rome" and all. Either way I don't knock using semis in your imports since everyone is different but just explaining that it's a normal thing for some devs and not exactly violating the spec.

For a list of some of the soms popular libraries in off of the top of my head:

Redux:
https://github.com/reduxjs/redux

React Redux:
https://github.com/reduxjs/react-redux/tree/master/test

CSSinJS:
https://github.com/cssinjs/jss

React Router (not currently but for most of its life):
https://github.com/ReactTraining/react-router/blob/v3/modules/IndexRedirect.js

Edit: crossing out terrible misinformation.

@andrewbranch
Copy link
Member

@rob2d Unless I’m really missing something, every example you just sent doesn’t use semicolons at all.

In my memory, eslint-config-standard, which prefers no semicolons, was really popular with many React folks in the early days, and more recently eslint-config-react-app I think has become a de-facto choice.

To be clear, I have absolutely zero judgement on how people format their code—I want you to put semicolons wherever they make you happy, no more and no less—it’s just that I feel like we’re unlikely to add additional heuristics to TypeScript to support preferences that are atypical and/or more work to test for. That’s why I asked for examples—not because I don’t believe you or think that’s a weird preference to have, but rather because I want to try to form an idea of how common this is by looking at the size of projects that adopt such a convention.

@rob2d
Copy link

rob2d commented Aug 5, 2019

well, I put some links there? Seems you did not click them if you are making this statement. I mean obviously in terms of that, check the React code and not the webpack code or course.

Edit: no, you definitely clicked.

@andrewbranch
Copy link
Member

I went into each repo you linked and found a random JS file, so possibly we’re looking at different files. Can you send a specific file where you see this convention? Here’s what I looked at:

@blikblum
Copy link

blikblum commented Aug 5, 2019

Seems you did not click them if you are making this statement.

I clicked on the links and they are not using semicolons at all

@rob2d
Copy link

rob2d commented Aug 5, 2019

Edit: upon further reflection, I hadn't noticed that they were either all or nothing, and I am an idiot 🙃 my whole life has just changed a bit thanks to this thread.

TLDR: you all have a very good point. Sorry about that.

@cancerberoSgx
Copy link

cancerberoSgx commented Aug 6, 2019

@andrewbranch - Thanks for working on the issue, I'm testing latest version to see if this issue was solved in PrinterOptions which has a property exactly for this which was working in the past so we developers can have control . Will update with results here. Left some questions / opinions #31801 (comment) thanks

@mjomble
Copy link

mjomble commented Aug 6, 2019

It might make more sense to have this discussion on #31801

@RyanCavanaugh
Copy link
Member

@cancerberoSgx replying to your pre-edit content, our belief was that implementing this as a heuristic would actually be the best behavior, since the tool would just "do the right thing" with no necessary configuration from the user. It seems it'd be better if TS simply inserted semicolons in files that seemed to use semicolons so that users didn't have to go find the configuration option.

@cancerberoSgx
Copy link

@RyanCavanaugh I give my feedback #31801 so there's less noise, thanks - ("do the right thing" sounds strange particularly if in a platform extendible by third parties with their own opinions- I think "the right thing" is to consistently support any code style that the JavaScript language specification allows any user to write their code with (since there's no standard for that and I don't think airbnb style guide or any other is even a defacto standard on this regard.Currently semicolons behavior and settings API is inconsistent with the rest of code format related semantics)

@finscn
Copy link

finscn commented Nov 22, 2021

any news?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue 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.