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

Support max-line-length in organize imports #22991

Open
mhegazy opened this issue Mar 29, 2018 · 38 comments
Open

Support max-line-length in organize imports #22991

mhegazy opened this issue Mar 29, 2018 · 38 comments
Labels
Domain: Formatter The issue relates to the built-in formatter Domain: Organize Imports Issues with the organize imports feature Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript
Milestone

Comments

@mhegazy
Copy link
Contributor

mhegazy commented Mar 29, 2018

Referenced in #22974

By the way, I ran "organize imports" on about 800 files. Works really well other than this problem and it puts imports that were previously on multiple lines onto one... so that will break my linting rule for "max-line-length". Luckily it's easy to write a script to investigate and break them back onto multiple lines 😄

@mhegazy mhegazy added Suggestion An idea for TypeScript Domain: Organize Imports Issues with the organize imports feature Domain: Formatter The issue relates to the built-in formatter labels Mar 29, 2018
@mhegazy
Copy link
Contributor Author

mhegazy commented Mar 29, 2018

I would say the correct fix here is to let the formatter split lines greater than certain length.

@MartinMa
Copy link

MartinMa commented May 7, 2018

This would be awesome.

So that this:

import { AfterContentInit, Component, ContentChildren, Directive, EventEmitter, HostBinding, Input, ModuleWithProviders, NgModule, OnInit, Optional, Output, QueryList, ViewEncapsulation, forwardRef } from '@angular/core';

becomes this:

import {
  AfterContentInit,
  Component,
  ContentChildren,
  Directive,
  EventEmitter,
  HostBinding,
  Input,
  ModuleWithProviders,
  NgModule,
  OnInit,
  Optional,
  Output,
  QueryList,
  ViewEncapsulation,
  forwardRef
} from '@angular/core';

Thus, respecting max-line-length (tslint).

@pelotom
Copy link

pelotom commented May 8, 2018

The auto-importer also breaks the ordered-imports TSLint rule, which specifies how imports should be broken up into groups and how they should be sorted. It would be nice if it were possible to enable/disable various parts of this organize imports command; in my case I want to disable everything except removing unused imports, because everything else is covered by a combination of TSLint and Prettier.

@mhegazy
Copy link
Contributor Author

mhegazy commented May 8, 2018

@pelotom that is tracked by #23366

@pelotom
Copy link

pelotom commented May 8, 2018

@mhegazy That issue seems to be specifically about the ordering of named imports within a single statement, but doesn't cover

  • ordering of different import statements relative to one another
  • groups of imports

or am I missing something?

@mhegazy
Copy link
Contributor Author

mhegazy commented May 8, 2018

which part you are saying conflict with the tslint rule? i thought the problem is that within the same import declaration, named import bindings are ordered differently by tslint (case insensitive) and by tsserver (case sensitive). are there other issues?

@pelotom
Copy link

pelotom commented May 8, 2018

@mhegazy TSlint also allows configuring the order of import statements relative to one another:

You may set the "import-sources-order" option to control the ordering of source imports (the "foo" in import {A, B, C} from "foo").

Possible values for "import-sources-order" are:
-"case-insensitive': Correct order is "Bar", "baz", "Foo". (This is the default.)
-"lowercase-first": Correct order is "baz", "Bar", "Foo".
-"lowercase-last": Correct order is "Bar", "Foo", "baz".
-"any": Allow any order.

And it has an option for configuring groupings of import statements:

You may set the "grouped-imports" option to control the grouping of source imports (the "foo" in import {A, B, C} from "foo").

Possible values for "grouped-imports" are:

  • false: Do not enforce grouping. (This is the default.)
  • true: Group source imports by "bar", "../baz", "./foo".

That is, it creates newline-separated groups like this:

import 'a';
import 'b';

import '../a';
import '../b';

import './a';
import './b';

Currently Organize Imports destroys these groupings and mashes them all together.

@mhegazy
Copy link
Contributor Author

mhegazy commented May 8, 2018

I see, thanks for sharing. i do not think such level of configurability will be in in organize imports the server supports. i think the best solution here is to not use organizeImports, and instead have tslint-vscode-plugin fix your lint errors on save.

@pelotom
Copy link

pelotom commented May 8, 2018

@mhegazy

i think the best solution here is to not use organizeImports, and instead have tslint-vscode-plugin fix your lint errors on save.

Unfortunately that's not viable for removing unused imports, because the vscode plugin for TSLint can't apply rules which require type checking. Besides that, the TSLint rule for no-unused-variable has myriad problems and @egamma has advised people to use TypeScript's native noUnusedLocals and noUnusedParameters instead. See https://github.com/Microsoft/vscode-tslint/issues/185, https://github.com/Microsoft/vscode-tslint/issues/70.

Is there any chance that a "remove unused imports" command could be added to the language server?

@mhegazy
Copy link
Contributor Author

mhegazy commented May 8, 2018

apply rules which require type checking

that has nothing to do with type checking. the organize imports does not require full type-checking, nor does it require semantic state outside the current file, it uses the find-all-refs logic.

Besides that, the TSLint rule for no-unused-variables has myriad problems and @egamma has advised people to use TypeScript's native noUnusedLocals and noUnusedParameters instead.

i think we are talking about two different things here. 1. what APIs needed from the TypeScript language service to make tslint better, and 2. what level of configuration is needed in organize imports feature

For 1, we would be happy to add additional APIs to enable tslint to be faster/more efficient. for 2, as i noted, the value of adding more configuration drops fairly quickly vs the value added.

I would say start a tslint issue, and we will talk o the tslint core team about what APIs we can enable to simplify this scenario.

Is there any chance that a "remove unused imports" command could be added to the language server?

There is already a suggestion for that, that is marked as unused (see #22361), and it comes with a quickfix to remove it, with quickFix all option on it. so an IDE could query for suggestions, and apply all fixes on save if that is needed. not specific to imports though, all declarations.

@pelotom
Copy link

pelotom commented May 8, 2018

that has nothing to do with type checking. the organize imports does not require full type-checking, nor does it require semantic state outside the current file, it uses the find-all-refs logic.

I'm talking about the TSLint no-unused-variable rule. I have no idea why it claims to require type checking, but it does, and type checking rules cannot be run inside VSCode (see the issues I linked). My point is just that there's no way as of right now to remove unused imports in VSCode with TSLint, which is why I was hoping to be able to use the Organize Imports feature to do it :)

There is already a suggestion for that, that is marked as unused (see #22361), and it comes with a quickfix to remove it, with quickFix all option on it. so an IDE could query for suggestions, and apply all fixes on save if that is needed. not specific to imports though, all declarations.

Great, looks like exactly what I want!

@Rush
Copy link

Rush commented Feb 6, 2019

Are there any workarounds for this issue?

@ZackMFleischman
Copy link

@Rush nothing I know of that doesn't require a small investment of scripting. I use saved vim macros to run the organize imports function and then do a second pass to fix anything that's too long and break it up according to regex rules. This works for one off files while editing.

I imagine you could do something similar in the scripting language of your choice over the entire project.

@MrDesjardins
Copy link

Our team has the same issue. We had to turn off the organize import feature. We are using Prettier which suit us well for formatting the code, however, it gets in conflict with the organize import when it is used to be fired when a file is saved.

The idea of having the organize import on save it to automatically have the unused import taken out without having human interaction. However, having the organize import and Prettier make the code to be on a single line once organized, and on multiple lines when Prettier is cleaning its part of the code. I haven't investigated why, but prettier seems to be executed first, which cause the code to be formatted correctly but then VsCode organizes and put back into a single line the import, hence causing issues of consistency.

@tfburton
Copy link

I'm sad that I just had to turn off organize imports. My team doesn't want a lint rule for the imports, but I like them to be organized. They are ok with me organizing them, but they don't want to be bothered with it. I was depending on this feature until I couldn't get multi-line imports to be happy.

@crashmstr
Copy link

Seems like this quickly got off the topic of line length and moved on to ordering of imports :(
There was a similar item that was marked as a duplicate of this one, but is there still attention here for just the concern over being able to limit line length when using organize imports? Line length and ordering of the imports seems like they should be two separate topics.

@ZackMFleischman
Copy link

ZackMFleischman commented Mar 23, 2019 via email

@RyanCavanaugh RyanCavanaugh added Experience Enhancement Noncontroversial enhancements and removed In Discussion Not yet reached consensus labels Apr 1, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Apr 1, 2019
@zpdDG4gta8XKpMCd
Copy link

tslint has an option to ignore max line in imports
you guys must be joking all around

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Apr 2, 2019

you scare me people:

image

https://palantir.github.io/tslint/rules/max-line-length/

PS

and the reason why you scare me because an issue that is in a domain of tslint in the first place (and already solved there btw), gets pulled out for the next sprint

@alexey-kozlenkov
Copy link

@Aleksey-Bykov do you confuse tslint rule with typescript language service feature?

@dsherret
Copy link
Contributor

dsherret commented Apr 2, 2019

@Aleksey-Bykov it's certainly in the domain of the language service's formatter, but not necessarily a feature that will be implemented. No need to be so spooked out.

(IMO, time would probably be better spent leaving this to other code formatters)

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Apr 2, 2019

let me say this:

  • we are still talking about the max-length problem (as the title suggest), don't we?

  • if so, then this problem simply does not exist, since tslint can ignore it

  • if no, i personally don't want any formatting to the imports, because:

    • i have tons of them, and each line counts, so i wish they were as flat as they can be, because...
    • ... i hate scrolling through them
    • ... and i stopped caring about imports about 2 years ago since
      1. auto imports
      2. auto imports cleanup
      3. auto imports sorting
  • so i simply don't look at them and i am glad i don't have to, and it will be a bad day if they start getting bloated because nicely formatted

@RyanCavanaugh
Copy link
Member

@Aleksey-Bykov When we look at issues with no thumbs up, you go into those issues and complain about it, but when we look at issues with 27 thumbs ups, you go into those issues and poo-poo their value too. This doesn't feel like good-faith engagement and I think it'd be best if you were more intentional about where you choose to weigh in on things.

@crashmstr
Copy link

@Aleksey-Bykov I don't want to remove or make exceptions for the tslint rule if at all possible, but I appreciate that I could and that for some, that may be their preferred solution.

I would like if VS Code could accommodate a tsline line length rule (or rule specified in VS Code) that does not have exceptions for imports, because for now I manually run the command to organize imports then edit for length (as that would not work for organize on save unless I do make exceptions for the tslint rule).

@zpdDG4gta8XKpMCd
Copy link

@RyanCavanaugh i apologize for being a stink bug, this is what i am, this is what i do, didn't mean to pick on you

@dsherret
Copy link
Contributor

dsherret commented Apr 2, 2019

@Aleksey-Bykov you've made your preference clear. Some other peoples' preference is for all code to fit within their screen's width to prevent needing to horizontally scroll regardless of whether they are using tslint—having the import statements automatically format would make the editing experience a little nicer for those people because they wouldn't need to fix this every time they run organize imports.

If anything, I think it would be more productive to ask for this to be configurable (which it likely will be if implemented) so you can turn it off.

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Apr 2, 2019

@dsherret i see, ok, although it's not what the original problem is about (and because of the fact it is not), i buy it

@dsherret
Copy link
Contributor

dsherret commented Apr 2, 2019

@Aleksey-Bykov I think it's related to the original problem (I'm the one quoted in it). This isn't really tslint related... think about what the motivation for that tslint rule is and transitively that's what the original post is about.

Additionally, the API and tsserver don't care about tslint. This feature if implemented would likely be a setting specified on FormatCodeSettings or its base (see here). Then vscode and other editors can provide that value however they please.

Anyway, this is a bit of a digression... (my apologies)

@vinagreti
Copy link

vinagreti commented Aug 26, 2019

VSCode Workaround for those who are having problem with format on save + sort import You can save the file, wait for it to be sorted, then you can break the long import line and than use the "Save without Formatting" option of vscode. Hit ctrl+p and type Save without Formatting.

@bduffany
Copy link

bduffany commented Apr 3, 2020

If you're like me, you like Prettier and VS Code but don't like how they clash when handling imports.

I've found a solution for VS Code that's been working very nicely, keeping the super fast formatting speed of Prettier while keeping imports tidy using eslint-plugin-import.

Check out the .eslintrc, .eslintignore, .vscode/settings.json, .prettierrc, and any devDependencies containing "typescript" or "eslint" in .package.json of https://github.com/bduffany/nextjs-app-template

@laurensnl
Copy link

I spent so much time getting it right (and failed!)... Thank for sharing your config @bduffany! Your solution seems to work perfectly!

@Elias-Graf
Copy link

Elias-Graf commented Apr 12, 2020

I haven't read through everything and don't get why there isn't an option for this yet. I'm not using prettier (So I think I'm using the default formatter? It's all a bit confusing when you're not completely into this kind of stuff) but es-lint with the typescript plugin. Couldn't we just say that we make a setting (for example: "source.organizeImportsMaxLen": 100) and break imports into one per line? Just provide the option and people can turn it on or off if they please.

Additionally, I'd suggest if you want something different like filing the line and only breaking when the import doesn't fit on it, you should have to use a custom formatter.

What I mean:

import {bla, bla, bla, bla
    bla, bla, bla, bla, bla,
    bla} from './bla';

This is also affecting exports. My multi-line export statement is being turned into one line:

export {
	Core,
	TokenKeyword,
	TokenSymbol,
	Token,
};

is being turned into a single line. Funny thing, export default {...}; does not have the same issue.

I'm also using "source.fixAll": true" if that makes a difference. Hopefully, I got everything correct and didn't make a complete fool of myself :D

Edit:
Why not just add a few more options so everyone can configure this as they pleases:

  • sortImports
  • deleteUnusedImports
  • importLineMode (I'd suggest options like: auto, mutli-line, single-line, manual / off)

@daidodo
Copy link

daidodo commented Apr 13, 2020

Hi, for those who is still struggling on those issues, please try JS/TS Import Sorter which I believe covers all following topics discussed here:

  • Max-line-length
  • TSLint/prettier compatible
  • Groups of imports
  • Removal of unused imports

If you do find bugs or features not supported, I'm happy to accept open issues as a maintainer.

Thanks!

@Elias-Graf
Copy link

@daidodo Works like a treat well done! It's also really configurable.

Question is, maybe this should work out of the box but it's not up to me to decide this.

@tommueller
Copy link

Any updates on this?

@thinkjson
Copy link

We ended up just using both organize imports and prettier on save.

@Bessonov
Copy link

I vote for support "one import/export statement per line with trailing comma", like:

import {
  AfterContentInit, <-- one statement per line
  Component,
  ContentChildren,
  Directive, <-- trailing comma, so additions after this line doesn't affect this line
} from '@angular/core';

instead of max-line-length property. This plays nicely with diffing, merging and code review.

@Rush
Copy link

Rush commented Jan 22, 2022

Clearly one size doesn't fit all so why even vote? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Formatter The issue relates to the built-in formatter Domain: Organize Imports Issues with the organize imports feature Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests