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

ESLint server only looks at first entry in context.only array sent along with textDocument/codeAction #1905

Open
mrnugget opened this issue Jul 17, 2024 · 4 comments
Labels
info-needed Issue requires more information from poster

Comments

@mrnugget
Copy link

At Zed we allow users to set up multiple code actions to be executed on save.

That breaks with the ESLint language server because the server only looks at the first entry of the context: { only: [] } array sent along with textDocument/codeAction requests.

Example: when we sent a request like this

{
  "jsonrpc": "2.0",
  "id": 140,
  "method": "textDocument/codeAction",
  "params": {
    // ...
    "context": {
      "diagnostics": [
        // ...
      ],
      "only": ["source.organizeImports", "source.fixAll.eslint"]
    }
  }
}

Then the ESLint server will only look at "source.organizeImports" and ignore the source.fixAll.eslint. And that has disastrous results: the server will add "disable" comments when executing organizeImports — which is not what users want most of the time.

@hmttrp tracked the behavior down to the code here:

const only: string | undefined = params.context.only !== undefined && params.context.only.length > 0 ? params.context.only[0] : undefined;

I'm not sure what exactly the intention behind the check is (I don't understand why not all entries are checked), but I think a fix would be to turn the booleans here

const isSource = only === CodeActionKind.Source;
const isSourceFixAll = (only === ESLintSourceFixAll || only === CodeActionKind.SourceFixAll);

into checks whether the array contains an item of that kind (and not just whether the first item has that kind).

@dbaeumer
Copy link
Member

This is a problem with the Zed client. The ESLint server registers for the following code action kinds

https://github.com/microsoft/vscode-eslint/blob/main/server/src/eslintServer.ts#L224

It should not be called with a code action kind of organize imports since this is nothing it has been registered for.

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Jul 30, 2024
@mrnugget
Copy link
Author

That's how I ended up fixing it: zed-industries/zed#14666

Ultimately your call, of course, but I think being more robust against input here and not executing different code actions would make future debugging easier.

@dbaeumer
Copy link
Member

I agree with that. But a client should not send unnecessary code actions requests for performance reasons.

@mrnugget
Copy link
Author

Yeah, 100%! That was a bug on our side, definitely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info-needed Issue requires more information from poster
Projects
None yet
Development

No branches or pull requests

2 participants