jose-elias-alvarez / nvim-lsp-ts-utils

Utilities to improve the TypeScript development experience for Neovim's built-in LSP client.
The Unlicense
438 stars 18 forks source link

[BUG] TSLspImportAll function doesn't work with JavaScript #99

Closed LandonSchropp closed 2 years ago

LandonSchropp commented 2 years ago

FAQ

Issues

Neovim Version

v0.6.0

Steps to reproduce

  1. Install LunarVim using the installation script. (I don’t believe this touches the existing Neovim config, but it’s probably better to have it backed up just in case.)

    INSTALL_SCRIPT="https://raw.githubusercontent.com/lunarvim/lunarvim/rolling/utils/installer/install.sh"
    (yes y | LV_BRANCH=rolling bash <(curl -s "$INSTALL_SCRIPT")) || true
  2. Open ~/.config/lvim/config.lua and replace it with the following config.

    lvim.plugins = {
     { "jose-elias-alvarez/nvim-lsp-ts-utils" },
    }
    
    lvim.lsp.on_attach_callback = function(client)
     print("on_attach_callback")
    
     if client.name ~= "tsserver" then
       return
     end
    
     local ts_utils = require("nvim-lsp-ts-utils")
    
     ts_utils.setup({
       auto_inlay_hints = false,
       update_imports_on_move = true,
     })
    
     ts_utils.setup_client(client)
    end
  3. Open up LunarVim (lvim) and run :PackerSync. Then close LunarVim. (This step might not be necessary, but I'm adding it just in case.)
  4. Open up an existing JavaScript/TypeScript project and delete an import from a file.
  5. Run :TSLspImportAll. No files will be imported. A message will be printed that says No code actions available.

Expected behavior

The deleted function show be re-added.

Actual behavior

The deleted import is not re-added.

Debug log

No response

Help

Yes

Implementation help

I would definitely need some guidance on how to fix the problem, but I'm happy to help out.

LandonSchropp commented 2 years ago

Please let me know if the repro steps are insufficient. I can create an example JavaScript repo if necessary.

jose-elias-alvarez commented 2 years ago

Thanks! Installing LunarVim was way easier than I imagined.

I tried this out on an existing TypeScript project and the command works as expected. Are you able to reproduce this without LunarVim, too? If you could put together an example repo, it would be a big help.

LandonSchropp commented 2 years ago

The LunarVim folks have done a great job making the process easy. They're approaching a 1.0 release, so I think they're really trying to make things simple. 🙂

Sure, I'll see if I can get something put together as an example. Maybe there's a weird interaction with my project config or TypeScript version? I'll debug and see what I come up with.

Thanks again for the help!

jose-elias-alvarez commented 2 years ago

Sounds good! TypeScript version is my top suspect. For reference, I'm using 4.5.4.

LandonSchropp commented 2 years ago

I tried uninstalling my local tsserver, and instead, I used the installation through nvim-lsp-config. Unfortunately, I'm seeing the same result. I'm still working on the repro repo, but I thought I'd mention this.

Screen Shot 2021-12-15 at 3 13 27 PM
LandonSchropp commented 2 years ago

Okay, I've created an example repo.

I tried the command with both TypeScript and Javascript. Typescript seems to be working great, but JavaScript isn't working.

jose-elias-alvarez commented 2 years ago

Okay! I think I've got it. Basically, the way :TSLspImportAll works is to get diagnostics from the current buffer, filter to keep only diagnostics related to missing imports, then get and run code actions to add all missing imports. By default, TypeScript doesn't check JavaScript files, so no diagnostics are emitted.

By telling the TypeScript compiler to check JavaScript files with the following tsconfig.json, the function works:

{
  "compilerOptions": {
    "target": "es2016",
    "module": "commonjs",
    "allowJs": true,
    "checkJs": true
  }
}

Of course, modifying tsconfig.json may be a no-go on work projects, but there's nothing we can without the diagnostics.

Edit: I'll add a mention of this to the documentation. The "correct" solution is to support this upstream, and I see that typescript-language-server actually added support in typescript-language-server/typescript-language-server#318. I'll test this out to see how it compares to our solution, though I'll mention that ours has a few additional features that make it useful.

LandonSchropp commented 2 years ago

Awesome! Thank you for digging into this. 🙂 I'll give this a shot tomorrow to confirm that this solves the issue for me.

LandonSchropp commented 2 years ago

That did the trick! Thanks again for all the help.

LandonSchropp commented 2 years ago

Hmm, well there is a big downside to the jsconfig.json solution. The checkJs parameter seems to be required. However, enabling it annoying starts checking JavaScript files for TypeScript type declarations.

Screen Shot 2021-12-16 at 2 07 26 PM

It would be awesome if this could be handled with the upstream solution to avoid this issue. 🙂

jose-elias-alvarez commented 2 years ago

I read through the documentation for these source actions but wasn't able to get them to work with Neovim, most likely because they're meant to run on save. I also can't say for sure whether they'd work for JavaScript, either.

In your situation, I'd try to find more information about how to run these actions in Neovim and try to set up a solution. Our implementation requires diagnostics and is Neovim-specific but enables a few handy features, like import priority and prompting the user when there are multiple options available.

LandonSchropp commented 2 years ago

I agree your solution sounds better. Also, finding a way to integrate these actions seems a bit beyond my expertise here.

As an alternative, do you know if there's any way to suppress the TypeScript type errors in the Neovim UI? I'd definitely like to leave on the other LSP indicators (such as null-ls), but I don't actually care about the TypeScript-specific errors.

jose-elias-alvarez commented 2 years ago

This suppresses the display of diagnostics in the main window (signs, virtual text, and highlights) but will still show up when using Lualine and other consumers of vim.diagnostic.get:

    -- inside on_attach
    if client.name == "tsserver" then
        vim.diagnostic.disable(bufnr, vim.lsp.diagnostic.get_namespace(client.id))
    end

Not sure if there's a better solution than that. Clearing out the diagnostics completely isn't an option, since then :TSLspImportAll won't have access to them and we're right back where we started.

jose-elias-alvarez commented 2 years ago

I've added a note to the documentation about the interaction in 083c918f8a53c69361954e9d879113676e30c2c8, so I think that's all we can do for now. When I can, I'll look into the source code actions again and see how we can run them (or how to add support in Neovim if they're not yet supported, which is what I suspect). Thanks for your patience and detailed reports!

LandonSchropp commented 2 years ago

This suppresses the display of diagnostics in the main window...

This is a pretty decent workaround. It's a bummer that it doesn't work everywhere, but it's better than nothing.

The main downside of using checkJs is I'm not sure it's going to work for team projects in the future (although it's fine for me now).

When I can, I'll look into the source code actions again and see how we can run them (or how to add support in Neovim if they're not yet supported, which is what I suspect).

That'd be awesome. It'll be cool to get this working at some point in the future. Would it be possible to request a ticket to track this so I can subscribe for updates?

Thanks for your patience and detailed reports!

Thank you for all the time and energy, as well as for creating several awesome tools I use every day. 🙂

jose-elias-alvarez commented 2 years ago

Thanks for the kind words! I opened #100 to track progress on the new actions.

LandonSchropp commented 2 years ago

Cool, thanks!