FixAll on save doesn't pick up file change from organize imports

[mjbvz]

From microsoft/vscode#88131 (comment)

Repro

1. Clone https://github.com/JacksonKearl/eslint-organizeImports-codeactions

2. This sets:

   "editor.codeActionsOnSave": [
   	"source.organizeImports",
   	"source.fixAll.eslint",
   ],

3. Open index.ts and save the file a few times

Bug
Notice the trailing command being added and removed on each save

[mjbvz]

Here's the ESlint trace of what happens:

VS Code update the document using organize imports (removing the trailing comma)

[Trace - 12:35:49 PM] Sending notification 'textDocument/didChange'.
Params: {
    "textDocument": {
        "uri": "file:///Users/matb/projects/san/eslint-organizeImports-codeactions/index.ts",
        "version": 33
    },
    "contentChanges": [
        {
            "range": {
                "start": {
                    "line": 6,
                    "character": 14
                },
                "end": {
                    "line": 6,
                    "character": 15
                }
            },
            "rangeLength": 1,
            "text": ""
        }
    ]
}

VS Code request code actions for fix all

[Trace - 12:35:49 PM] Sending request 'textDocument/codeAction - (98)'.
Params: {
    "textDocument": {
        "uri": "file:///Users/matb/projects/san/eslint-organizeImports-codeactions/index.ts"
    },
    "range": {
        "start": {
            "line": 0,
            "character": 0
        },
        "end": {
            "line": 16,
            "character": 0
        }
    },
    "context": {
        "diagnostics": [],
        "only": [
            "source.fixAll.eslint"
        ]
    }
}

ESlint updates it's diagnostics

[Trace - 12:35:49 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///Users/matb/projects/san/eslint-organizeImports-codeactions/index.ts",
    "diagnostics": [
        {
            "message": "Insert `,`",
            "severity": 1,
            "source": "eslint",
            "range": {
                "start": {
                    "line": 6,
                    "character": 14
                },
                "end": {
                    "line": 6,
                    "character": 14
                }
            },
            "code": {
                "value": "prettier/prettier",
                "target": "https://github.com/prettier/eslint-plugin-prettier#options"
            }
        }
    ]
}

ESLint then returns no code actions

[Trace - 12:35:49 PM] Received response 'textDocument/codeAction - (98)' in 23ms.
Result: []

[dbaeumer]

I looked into this and for me it looks like this: TypeScript and ESLint do have two conflicting fixes for the organize import.

• Typescript wants this (e.g. one long line):

import { LongName, ReallyQuiteSuperLongName, ReallyQuiteSuperMegaLongName, ReallySuperLongName, ReallyVeryQuiteSuperMegaLongName, SuperLongName } from './utils'

• and ESLint wants this (one import per line)

import {
	LongName,
	ReallyQuiteSuperLongName,
	ReallyQuiteSuperMegaLongName,
	ReallySuperLongName,
	ReallyVeryQuiteSuperMegaLongName,
	SuperLongName,
} from './utils'

I don't see how ESLint can address this.

@mjbvz and @JacksonKearl can you please outline what in your opinion ESLint should do to avodi this?

[dbaeumer]

This conflicting behavior results in the fact that the import switches on every Save.

[dbaeumer]

There is something else wrong as well since I see this in the console:

[Error - 3:04:36 PM] Failed to apply command: eslint.applyDisableLine
[Error - 3:04:36 PM] Failed to apply command: eslint.applyDisableFile

[dbaeumer]

The problematic piece seems to be

	"editor.codeActionsOnSave": [
		"source.organizeImports",
		"source.fixAll.eslint",
	],

I always thought the format is

	"editor.codeActionsOnSave": {
		"source.organizeImports": true,
		"source.fixAll.eslint": true,
	},

[dbaeumer]

Change the settings to

	"editor.codeActionsOnSave": {
		"source.organizeImports": true,
		"source.fixAll.eslint": true,
	},

basically avoid printing the Failed to apply command: message but make the editor flicker due to the problem that the actions run on save have conflicting goals (e.g. TypeScript and ESLint). I will change the code in ESLint to not fall over

"editor.codeActionsOnSave": [
		"source.organizeImports",
		"source.fixAll.eslint",
]

[dbaeumer]

To make things clear: I fixed the fact that the setting was not read correctly. However the sertup itself still doesn't result in a user friendly behaviour :-). But IMO there is nothing ESLint can do to address this.

[JacksonKearl]

The idea with #88131 was to allow "editor.codeActionsOnSave" to be an array so you can configure "organize the imports first, then run the code through eslint":

"editor.codeActionsOnSave": [
		"source.organizeImports",
		"source.fixAll.eslint",
]

If instead you use:

	"editor.codeActionsOnSave": {
		"source.organizeImports": true,
		"source.fixAll.eslint": true,
	},

The fixAll gets run first, then the organize imports is run so the end state isn't eslint conformant.

If however we use the array syntax, it should run organize imports, converting the working copy to have organized imports but not be eslint conformant, then immediately run eslint, preserving the order of the imports but not their formatting, and making the document eslint conformant. And only then should it be persisted to disk.

What I'm observing instead is that I save one time and the organizeImports output is written to disk. Then I save again and the eslint output is written to disk. And it keeps alternating between these two.

@mjbvz and @JacksonKearl can you please outline what in your opinion ESLint should do to avodi this?

All eslint should need to do is the same formatting it has always done, the input will be organizeImports conformant but the output doesn't need to be.

@mjbvz maybe organize imports could be changed to not care about formatting, only order? So if the imports are in correct order it doesn't touch them and we don't get flicker.

[mjbvz]

Thanks for the details @JacksonKearl. That is correct

@dbaeumer I was surprised to see that changing editor.codeActionsOnSave setting to an array caused issues for eslint. VS Code should ask eslint for code actions for source.fixAll.eslint on save. Why does eslint need to read this setting itself?

[dbaeumer]

@mjbvz the current reason left is settings migration for which we don't have any store in VS Code except that the extension does it itself.