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

breaking: codeActionsOnSave #194861

Closed
ilteoood opened this issue Oct 5, 2023 · 15 comments
Closed

breaking: codeActionsOnSave #194861

ilteoood opened this issue Oct 5, 2023 · 15 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug candidate Issue identified as probable candidate for fixing in the next release insiders-released Patch has been released in VS Code Insiders verified Verification succeeded

Comments

@ilteoood
Copy link

ilteoood commented Oct 5, 2023

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

  • VS Code Version: 1.83.0
  • OS Version: *

Steps to Reproduce:

With version 1.83.0, codeActionsOnSave accepts only an object. Unfortunately for my use case is deal-breaker because I would like to apply format and sorting before the lint. With the object configuration, it is not possible anymore.

This is my actual configuration:

"editor.codeActionsOnSave": {
    "editor.formatOnSave": "always",
    "source.organizeImports": "always",
    "source.sortImports": "always",
    "source.fixAll": "always"
  }

There is an old issue that describes my exact same case: #88131.

Do you have any workaround to achieve the same result, or can you please consider rolling back the array configuration?

@gjsjohnmurray
Copy link
Contributor

Source seems to indicate that array is still allowed:

const codeActionsOnSaveSchema: IConfigurationPropertySchema = {
oneOf: [
{
type: 'object',
properties: codeActionsOnSaveDefaultProperties,
additionalProperties: {
type: ['string', 'boolean']
},
},
{
type: 'array',
items: { type: 'string' }
}
],
markdownDescription: nls.localize('editor.codeActionsOnSave', 'Run CodeActions for the editor on save. CodeActions must be specified and the editor must not be shutting down. Example: `"source.organizeImports": "explicit" `'),
type: 'object',
additionalProperties: {
type: ['string', 'boolean'],
enum: ['always', 'explicit', 'never', true, false],
},
default: {},
scope: ConfigurationScope.LANGUAGE_OVERRIDABLE,
};

@gjsjohnmurray
Copy link
Contributor

Or maybe when 153335b from @justschen added type and additionalProperties in lines 55-59 above these overrode the oneOf array at lines 41+ ?

@ilteoood
Copy link
Author

ilteoood commented Oct 5, 2023

Or maybe when 153335b from @justschen added type and additionalProperties in lines 55-59 above these overrode the oneOf array at lines 41+ ?

This is possible. If I try with an array this is the result:
image

And no actions are performed (with the array filled with the proper actions)

@justschen justschen assigned justschen and unassigned mjbvz Oct 5, 2023
justschen referenced this issue Oct 5, 2023
* removed saveReason check for autosave

* modified setting values and enabling on autosave or not

* cleanup

* add default to false settings

* modified code action save settings

* changed default setting

* cleanup code and added backward compatibility

* aded minor comment and checking commit signing

* second check on commit signing (sorry)

* fix typo

* small bugfix and adding proper backward compatibility

* deprecate boolean values by converting them
@justschen
Copy link
Contributor

justschen commented Oct 5, 2023

Error might be coming from 153335b in saveParticipants (lines 283, where string[] was replaced) as well as what @gjsjohnmurray mentioned.

Interesting case - when there are Code Actions in the array, even with the error (saying Incorrect type...), do code actions still run?

@ilteoood
Copy link
Author

ilteoood commented Oct 5, 2023

Error is coming from 153335b in saveParticipants (lines 283, where string[] was replaced)

Interesting case - when there are Code Actions in the array, even with the error (saying Incorrect type...), do code actions still run?

I tried rolling back to my previous conf:
image

"editor.codeActionsOnSave": [
    "editor.formatOnSave",
    "source.organizeImports",
    "source.sortImports",
    "source.fixAll"
  ]

And unfortunately no actions are performed

image

@justschen
Copy link
Contributor

roger. looking at it rn, and draft PR is mentioned here. will take a look at this again first thing in the morning.

@ArturoDent
Copy link

@justschen
Copy link
Contributor

justschen commented Oct 5, 2023

@ArturoDent not sure if you are the OP, but I wasn't able to find a difference in current behavior vs. what happens in 1.82.

@ilteoood Do you have a repro of this for expected vs. current behavior? Assessing if there is a work around using the object configuration we have in 1.83 or not.

edit: still wasn't able to get a repro, but the main issue atm is that objects don't allow specific orderings. source.fixAll type source actions will always be executed first because of

if (CodeActionKind.SourceFixAll.contains(a)) {

@rcb4t2
Copy link

rcb4t2 commented Oct 5, 2023

@justschen Thanks for taking this on!

The difference in current behavior vs 1.82 is that in 1.83 array values are silently ignored, and code actions are not run on save

As you have mentioned, an object config is not suitable because we can't specify the order. This broke our dev environment, and we look forward to a fix. Thank you!

@Planeshifter
Copy link

Planeshifter commented Oct 5, 2023

Running into this issue as well, so it is very much appreciated @justschen that you are looking into this.

Specifically, we are using a combination of Prettier and ESLint for formatting and linting to enforce JS Standard Style, which depends on the order in which the actions are applied.

  "editor.codeActionsOnSave": ["source.formatDocument", "source.fixAll.eslint"],
  "editor.defaultFormatter": "esbenp.prettier-vscode",

Being able to format the document via Prettier and then applying ESLint fixes does not seem to be possible with the object notation right now.

With v1.83.0, when the array is used for codeActionsOnSave, one gets a Incorrect type. Expected "object". message in the settings.json file and no code actions are applied on save.

@ilteoood
Copy link
Author

ilteoood commented Oct 5, 2023

Hi @justschen, here is a repro. Consider this spec file: https://github.com/orchy-mfe/orchy-core/blob/main/packages/web-component/src/configuration-client/httpConfigurationClient.spec.ts

Here is the behaviour of vscode 1.82:
https://github.com/microsoft/vscode/assets/6383527/dbbea965-3d57-4393-87b4-77eff17da0e0

While this is the behaviour of vscode 1.83, without any change in the configuration:
https://github.com/microsoft/vscode/assets/6383527/6fd9ee90-3db2-4cf0-93d6-91df72ee297a

If I update the configuration to be an object, this is the final result:
https://github.com/microsoft/vscode/assets/6383527/0090e1ed-6b7e-495d-ab4d-becc65ac1588

As you can see, at each save imports are flickering but never really fixed.

@justschen
Copy link
Contributor

Candidate PR will just add back support for arrays for now! Under discussion atm if orderings from objects will be considered.

@bhavyaus bhavyaus added the bug Issue identified by VS Code Team member as probable bug label Oct 6, 2023
@bhavyaus bhavyaus added this to the September 2023 Recovery 1 milestone Oct 6, 2023
@bhavyaus bhavyaus added the candidate Issue identified as probable candidate for fixing in the next release label Oct 6, 2023
@justschen
Copy link
Contributor

justschen commented Oct 6, 2023

insiders PR: #194870
release branch PR: #194930

Things to test:

  1. Search in settings for Code actions on save
  2. edit in settings.json
  3. Test scenarios for explicit, always, and never when saving normally, and do so again when Auto Save is enabled for Window change and Focus Change, as well as when Auto Save is enabled after delay (in which case nothing would happen).
  4. Test scenarios for true and false in same conditions.
  5. Code Actions with the array (ensure no error, runs like true and false).
  6. Edge Cases: Scenarios like ESLint codeActionsOnSave not work #194978 where multiple types of source actions are used.

Example for arrays:

"editor.codeActionsOnSave": [
    "editor.formatOnSave",
    "source.organizeImports",
    "source.sortImports",
    "source.fixAll"
  ]

Runs when code actions are in the array, does not run when code actions are excluded from the array.

Known issues:

@kferch-teamworks
Copy link

kferch-teamworks commented Oct 9, 2023

Arrays for codeactionsonsave were what allowed the "Format Code Action", rohit-gohri.format-code-action to work. This was important because the array determines the order which you can't do with an object. This was broken with 1.83 change to object.

@justschen justschen added the insiders-released Patch has been released in VS Code Insiders label Oct 10, 2023
@andreamah andreamah added the verified Verification succeeded label Oct 10, 2023
@andreamah
Copy link
Contributor

verified on latest insiders

Version: 1.84.0-insider (user setup)
Commit: eb4f3c1
Date: 2023-10-10T07:26:46.161Z
Electron: 25.8.4
ElectronBuildId: 24154031
Chromium: 114.0.5735.289
Node.js: 18.15.0
V8: 11.4.183.29-electron.0
OS: Windows_NT x64 10.0.22621

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug candidate Issue identified as probable candidate for fixing in the next release insiders-released Patch has been released in VS Code Insiders verified Verification succeeded
Projects
None yet
Development

No branches or pull requests