Typescript: "source.organizeImports" is reordering imports and comments in polyfills.ts

[johncrim]

Issue Type: Bug

Given a simple angular project, created with ng new angular-repro, update .\.vscode\settings.json to:

{
  "editor.codeActionsOnSave": {
    "source.organizeImports": true
  }
}

Then edit the polyfills.ts file to either uncomment some of the imports or document why some polyfill imports are needed. For example, I added this at the end of the file:

/***************************************************************************************************
 * Zone JS is required by default for Angular itself.
 */
import 'zone.js/dist/zone'; // Included with Angular CLI.

/***************************************************************************************************
* APPLICATION IMPORTS
*/
// Needed to use dynamically compiled components in AOT/prod builds: https://github.com/angular/angular/issues/27584#issuecomment-446462051
import 'core-js/es7/reflect';

When I save the file, the text is reordered to:

/***************************************************************************************************
 * Zone JS is required by default for Angular itself.
 */
/***************************************************************************************************
* APPLICATION IMPORTS
*/
// Needed to use dynamically compiled components in AOT/prod builds: https://github.com/angular/angular/issues/27584#issuecomment-446462051
import 'core-js/es7/reflect';
import 'zone.js/dist/zone'; // Included with Angular CLI.

Which makes the comments less than useful, and is frustrating. The only ways to save the file without triggering this reformatting are:

1. Disable the line in settings.json (requires developers to know that this is causing the reformatting)
2. or close VS Code without saving.

VS Code version: Code 1.30.0 (c6e592b2b5770e40a98cb9c2715a8ef89aec3d74, 2018-12-11T22:29:11.253Z)
OS version: Windows_NT x64 10.0.17134

System Info

|Item|Value|
|---|---|
|CPUs|Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz (12 x 2904)|
|GPU Status|2d_canvas: enabled
checker_imaging: disabled_off
flash_3d: enabled
flash_stage3d: enabled
flash_stage3d_baseline: enabled
gpu_compositing: enabled
multiple_raster_threads: enabled_on
native_gpu_memory_buffers: disabled_software
rasterization: enabled
video_decode: enabled
video_encode: enabled
webgl: enabled
webgl2: enabled|
|Memory (System)|15.74GB (3.95GB free)|
|Process Argv|--folder-uri file:///c%3A/src/ease/WebUi|
|Screen Reader|no|
|VM|0%|

Extensions (24)

Extension|Author (truncated)|Version
---|---|---
ng-template|Ang|0.1.11
vscode-svgviewer|css|1.4.7
dart-code|Dar|2.21.1
flutter|Dar|2.21.1
githistory|don|0.4.4
xml|Dot|2.3.2
gitlens|eam|9.2.0
EditorConfig|Edi|0.12.5
json-tools|eri|1.0.2
todo-tree|Gru|0.0.109
svg|joc|0.1.4
docomment|k--|0.1.2
vscode-json5|mrm|1.0.0
csharp|ms-|1.17.1
PowerShell|ms-|1.10.1
vscode-typescript-tslint-plugin|ms-|0.2.0
debugger-for-chrome|msj|4.11.1
debugger-for-edge|msj|1.0.6
vscode-jest|Ort|2.9.2
tsimporter|pmn|1.2.14
partial-diff|ryu|1.4.0
code-settings-sync|Sha|3.2.4
stylelint|shi|0.47.0
quokka-vscode|Wal|1.0.174

[mjbvz]

Issue is that comments are not being moved along with imports. However there also cases where you really do not want the comments moved with the imports, such as with copyright header comments

Moving upstream to get more feedback

[weswigham]

The file leading comment should be moved with the import unless it's a "pinned" comment (a comment which starts with /*!).

[mjbvz]

See #22731. I am not in favor of regressing this as I do not see many codebases using pinned file headers

[weswigham]

@DanielRosenwasser @amcasey your opinions?

[johncrim]

If there were a way to easily disable reformatting a file, that would work here as a workaround; but this would still be an issue, and developers would have to discover the workaround.

Note that every angular project has a polyfills.ts file of this format. The expected use is that devs will comment out, or uncomment, imports based on their anticipated needs.

Leaving this as is guarantees frustration for any angular developer who has this feature enabled, and tries to edit the polyfills.ts file.

[johncrim]

My preference is: Don't re-order imports if comments are co-mingled between the imports.

[aaronatmycujoo]

Or group the comments above the line with the import statement?

[kayahr]

I'm not sure if this issue is just about re-ordered imports loosing their comments... I think the more important issue is that these imports must not be re-ordered at all because polyfills may need to be loaded in a specific order.

So while import organizing is a pretty useful feature for every other file there must be a way to prevent import reordering on this specific file.

Some suggestions:

• Add a new setting (i.e. source.organizeImports.exclude) with which we can specify file globs which are then excluded from import organizing.

• Accept comments for grouping imports and only reorder imports within the groups. So this:

  // Base polyfill
  @import "core-js";
  
  // Other polyfills
  @import "foo";     
  @import "bar";
  
  // Modules
  @import "b";
  @import "c";
  @import "a";
  

  becomes this:

  // Base polyfill
  @import "core-js";
  
  // Other polyfills
  @import "bar";
  @import "foo";     
  
  // Modules
  @import "a";
  @import "b";
  @import "c";
  

• Don't organize imports when there is a tslint:disable:ordered-imports comment in the file which is most likely there anyway to disable the corresponding tslint rule.

• Implement your own marker comment to disable import organizing. Maybe a tripple-slash-directive?

  /// <disable-organize-imports />
  @import "core-js";
  @import "foo";     
  @import "bar";
  

[kayahr]

Found a stinking workaround for the problem which may cause some trouble with some tools but so far it seems to work at least with tsc, vscode, tslint, webpack and webpack-dev-server:

import "01/../core-js";
import "02/../hammerjs";
import "03/../raf-polyfill";
import "04/../zone.js";
import { enableProdMode } from "@angular/core";

[jessetrinity]

Issue is that comments are not being moved along with imports. However there also cases where you really do not want the comments moved with the imports, such as with copyright header comments

This might be a bit outdated now. Today we do move comments when we organize imports, with the exception of the first comment because it could be the header. We could maybe be a little smarter with this by looking at blank lines so that we know the comment on the import here is separate from the header:

before organize

/*******************************************
* This is the header!
* I know the header is separate from the 'zzz' comment because there is a blank line between them!
*/

/**
* zzz
*/
import "zzz"

/**
* aaa
*/
import "aaa"

after organize

/*******************************************
* This is the header!
* I know the header is separate from the 'zzz' comment because there is a blank line between them!
*/

/**
* aaa
*/
import "aaa"

/**
* zzz
*/
import "zzz"

Otherwise it seems like you would not want the setting turned on in the first place if you need your imports to happen in a certain order, but configuring that on a per file basis would be a VSCode change.

@mjbvz Aside from this template, are there other cases where you would want to organize imports within separate sections?

[mjbvz]

No, I have not see other reports of this against VS Code

[jessetrinity]

@mjbvz Thanks. It looks to me like the ask here is for the ability to toggle organizeImports at the file level which is probably just a VSCode thing. Should we move this issue back to VSCode?

[mjbvz]

Possibly. VS Code's JS/TS support does not have per-file setting though so this would be a new concept. We'd also only implement if if there's enough user interest

I also wonder if a new directive such as // @ts-skip-organize-imports be a better fit for this? That would work across editors

[jessetrinity]

This would be a new concept for TypeScript as well. We only have compiler directives right now, not language service directives.

Our view on this at the moment is that a refactor request should not be sent if a refactor is wholly undesirable as it is in this case. We have a lot of refactors and quick fixes so allowing directives for them might quickly add up to a lot of not very meaningful requests to the server.

It would be good to get more feedback on the proposed idea of grouping imports though. Maybe we can do something clever with regions?

//#region group1
import "z"
import "y"
//#endregion

//#region group2
import "b"
import "a"
//#endregion

//#region group1
import "y"
import "z"
//#endregion

//#region group2
import "a"
import "b"
//#endregion

[simonhaenisch]

I've integrated organizeImports into a prettier plugin (see mentioned issue 👆) so that it runs as a preprocessor of the formatter. We've added support for an // organize-imports-ignore comment which stops prettier from running organizeImports on files that contain the comment. So that solves the whole "per-file basis" problem of this issue for us.

But there's still a problem with the order of comments:

// comment on top of line 1
import foo from "foo";
// comment on top of line 2
import bar from "bar";

console.log(foo, bar);

becomes

// comment on top of line 1
// comment on top of line 2
import bar from 'bar';
import foo from 'foo';

console.log(foo, bar);

It seems like the "line 1" comment is considered a file header (first comment of the file) and never moved?

import abc from 'abc';
// comment on top of line 1
import foo from "foo";
// comment on top of line 2
import bar from "bar";

console.log(abc, foo, bar);

👆 this one works as expected (comments are moved with the imports):

import abc from 'abc';
// comment on top of line 2
import bar from 'bar';
// comment on top of line 1
import foo from 'foo';

console.log(abc, foo, bar);

We could maybe be a little smarter with this by looking at blank lines

That's pretty much what I mean. I'm not saying it's wrong that the first comment doesn't get moved but i think that should only be the case if it's followed by a blank line. That's pretty common I would think.

/cc @mohsen1 have a look at https://github.com/microsoft/TypeScript/issues/29028#issuecomment-607520158

[johncrim]

Here's another standard angular case (eg the file is created by ng new) where the organizeImports behavior (again, in VS Code only) combined with autoformatting on save is causing major problems:

Given a test.ts file like:

// This file is required by karma.conf.js and loads recursively all the .spec and framework files
import 'zone.js/dist/zone';
import 'zone.js/dist/zone-testing';

import { getTestBed } from '@angular/core/testing';
import {
  BrowserDynamicTestingModule,
  platformBrowserDynamicTesting
} from '@angular/platform-browser-dynamic/testing';

Upon saving, the imports become:

// This file is required by karma.conf.js and loads recursively all the .spec and framework files
import { getTestBed } from '@angular/core/testing';
import {
  BrowserDynamicTestingModule,
  platformBrowserDynamicTesting
} from '@angular/platform-browser-dynamic/testing';
import 'zone.js/dist/zone';
import 'zone.js/dist/zone-testing';

This reordering causes tests to fail with an error message that is difficult to debug. This StackOverflow post exactly describes the problem that is caused when imports are re-ordered.

I would be happy if there were a comment or region text that could be used to disable re-ordering for a file, or portion of a file.
In short, organizeImports is placing ES6 imports

[johncrim]

Of the proposals in this issue, I would love to see something like:

/// file:disable-organize-imports

(obviously to skip organizing imports for the whole file)

_and_

/// <disable-organize-imports>
...
imports that won't be touched,
and imports before and after this block won't be moved before it or after it.
...
/// </disable-organize-imports>

I do really like @kayahr 's proposal:

Don't organize imports when there is a tslint:disable:ordered-imports comment in the file which is most likely there anyway to disable the corresponding tslint rule.

But since tslint is on its way out, I don't think it makes much sense to tie the syntax to tslint. Maybe an equivalent using an existing eslint directive would be the way to go.

[jessetrinity]

Disabling organize imports (and similar behaviors like formatting, see #18261) in a specific region makes the most sense as a TypeScript feature.

Disabling at the file level is something that should be implemented as a editor setting, as TS does not organize imports unless asked to by an editor. I don't believe VSCode even organizes imports on save unless you explicitly enable the feature via the editor settings. I imagine something like this:

{
  "editor.codeActionsOnSave": {
    "source.organizeImports": true,
    "ignore": [
        "polyfill.ts",
    ]
  }
}

[jessetrinity]

Closing this as the original issue is a VSCode settings limitation issue.