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

Organize Import creates whitespace that needs to be deleted manually #40736

Closed
jsgoupil opened this issue Sep 22, 2020 · 10 comments
Closed

Organize Import creates whitespace that needs to be deleted manually #40736

jsgoupil opened this issue Sep 22, 2020 · 10 comments
Assignees
Labels
Bug A bug in TypeScript Domain: Formatter The issue relates to the built-in formatter Domain: Organize Imports Issues with the organize imports feature Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fix Available A PR has been opened for this issue Help Wanted You can do this Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@jsgoupil
Copy link

jsgoupil commented Sep 22, 2020

TS Template added by @mjbvz

TypeScript Version: 4.1.0-dev.20200918

Search Terms

  • organize imports

  • VSCode Version: 1.49.1 (user setup)
  • OS Version: Windows_NT x64 10.0.19041

I am not 100% sure what is the exact scenario how to reproduce this, but it happens 90% of the time. When I press ALT+SHIFT+O (Organize Imports), I get plenty of spacing in my imports.
The rules for organizing these imports seem to be all over the place, which one ends up on their own line? etc.
I always have to clean up manually.

These imports:

import {
    ChangeDetectionStrategy,
    Component,
    ContentChild,
    EventEmitter,
    Input,
    Output,
    TemplateRef,
    AfterViewInit,
    ChangeDetectorRef,
    HostBinding,
    ElementRef,
    ViewChild
} from '@angular/core';
import { Resource, ClickableResource } from '@models/resource';
import { newGuid } from '@utility/string';
import { DragulaService } from 'ng2-dragula';

Become:

import {
    AfterViewInit, ChangeDetectionStrategy,







    ChangeDetectorRef, Component,
    ContentChild,







    ElementRef, EventEmitter,





    HostBinding, Input,
    Output,
    TemplateRef,




    ViewChild
} from '@angular/core';
import { ClickableResource, Resource } from '@models/resource';
import { newGuid } from '@utility/string';
import { DragulaService } from 'ng2-dragula';

Video of what is happeining.
spacing-incorrect.zip

Does this issue occur when all extensions are disabled?: Yes

@mjbvz
Copy link
Contributor

mjbvz commented Sep 23, 2020

Can you share a small project that causes this. I can't reproduce it

@jsgoupil
Copy link
Author

Try this:

import {
    ChangeDetectionStrategy,
    Component, ContentChild, EventEmitter, Input, Output, TemplateRef,
    AfterViewInit,
    ChangeDetectorRef,
    HostBinding,
    ElementRef,
    ViewChild
} from '@angular/core';
import { Resource, ClickableResource } from '@models/resource';
import { newGuid } from '@utility/string';
import { DragulaService } from 'ng2-dragula';

interface A extends ContentChild {}
interface B extends EventEmitter {}
interface C extends Input {}
interface D extends Output {}
interface E extends TemplateRef {}
interface F extends AfterViewInit {}
interface G extends ChangeDetectorRef {}
interface H extends HostBinding {}
interface I extends ElementRef {}
interface J extends Resource {}
interface L extends ClickableResource {}

@Component({
    selector: 'wm-table',
    templateUrl: 'table.component.html',
    styleUrls: ['table.component.scss'],
    providers: [DragulaService],
    changeDetection: ChangeDetectionStrategy.OnPush
})
export class TableComponent implements AfterViewInit {
}

@mjbvz mjbvz transferred this issue from microsoft/vscode Sep 24, 2020
@mjbvz mjbvz removed their assignment Sep 24, 2020
@mjbvz
Copy link
Contributor

mjbvz commented Sep 24, 2020

Thanks @jsgoupil!

@DanielRosenwasser Can we make sure this is scheduled for TS 4.1? (I have not see other reports so far, so I don't think we want to try fixing it in a 4.0 recovery build)

@DanielRosenwasser DanielRosenwasser added Domain: Formatter The issue relates to the built-in formatter Domain: Organize Imports Issues with the organize imports feature Bug A bug in TypeScript Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this labels Sep 24, 2020
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.1.1 milestone Sep 24, 2020
@jsgoupil
Copy link
Author

Maybe there should be something a little more predictable when it comes to either putting them all the imports on one line or putting them all on different lines? It seems quite random. Also, make sure to check how many lines end up under the last import and in order to stay consistent.

It had been happening forever, but I thought it would just get fixed one day :) Now that I'm doing major refactoring, this is happening a lot.

@andrewbranch
Copy link
Member

This bug is almost definitely caused by #36688. I’m not sure off the top of my head what the best fix is, but that’s the first place to look. @elibarzilay, I can take this one if you want. /cc also @a-tarasyuk.

@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Dec 11, 2020
@elibarzilay
Copy link
Contributor

@andrewbranch: I looked at this, and at the closed #40895, and I'm confused. It looks like #36688 is about preserving trivia(?) (comments & newlines, and maybe whitespace?) but I don't see how this would make sense when reordering things:

  • Wouldn't this get very messed up when there's an import line that looks like

    Foo, // blah blah

    since the comment will get dragged along with the next import (so will appear on whatever happens to be before it, "doubly shuffled")?

  • If the source has empty lines to separate parts of the imports, then those would also move in unexpected ways; even worse with comment lines to separate such parts.

Given this, I'd expect the reasonable option to be dropping all the extras, and maybe at most do the minor thing of putting newlines in the new text if the original had any newlines, but nothing else.

Clearly there's some brokenness in these cases with the extra empty lines, and I'm guessing that the #36688 doesn't account for lines being shuffled which leads to it, but even if this didn't happen, I don't see how the fixed thing would make sense given the above.

@andrewbranch
Copy link
Member

This should be fixed by #42630.

@andrewbranch
Copy link
Member

Looks like #41417 should have been marked as an exact duplicate of this, in fact.

@elibarzilay
Copy link
Contributor

Closing this, since as @andrewbranch notes, it's the same as #41417, and I also verified that #42630 resolves this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Formatter The issue relates to the built-in formatter Domain: Organize Imports Issues with the organize imports feature Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fix Available A PR has been opened for this issue Help Wanted You can do this Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

No branches or pull requests

7 participants