handler injection should be on tsserver client / buf only, not global

[folke]

I'm having some issues right now when I have a typescript file open and then open any other file.

There's a check somewhere in the code for filetype, but that's not enough.

Instead of injecting code in buf_request, it's better to automatically override the client's request method during on_attach. This way, the code will only ever run in a typescript client.

Suggested changes:

• require("nvim-lsp-ts-utils").setup{...} should not run inside on_attach, since that would setup multiple times when new buffers and / or workspaces are loaded
• during the on_attach, for client.name == 'typescript' we need to inject the code that overrides the client.request method
• that's pretty simple to do as in #12 and I have it running like this on my local branch
• enable_diagnostics can also run inside the attach, since we know the exact buffer that has the client

Apart from the above, there's a bug somewhere that probably calls handlers recursively in an endless loop, but I wasn't able to pinpoint yet where that happens. Seems to happen when I open a ts file and then open a lua file. Then switch back to the ts file and hover over a line with a code action. Probably related to setup being run multiple times.

If you're OK with the changes above, I can work on a PR for all of this.

Or if you have any other ideas, let me know :)

[jose-elias-alvarez]

Overriding the client's request handler sounds like an excellent improvement, and from briefly playing around with it, I can see that it would be straightforward (and that the only reason I haven't run into similar problems is because I don't work with multiple language servers in the same Neovim instance, apart from one-offs with JSON and YAML). It's a small breaking change to configs that are already using anything that relies on buf_request, but I think it's worth it to avoid leaking TypeScript-specific behavior between buffers. I'd be very happy to accept a PR for this.

I wonder whether running setup in on_attach is inherently an issue. Its current behavior is for the most part local to the specific buffer, apart from defining commands (today I learned that Vim has a -buffer flag for defining commands, so I can make those buffer-specific) and the import on completion autocommand group (which I'll fix, too).

Apart from those, where do you see problems with running setup multiple times?

[jose-elias-alvarez]

I tested this out more and can see that it's quite a serious issue, so I'm very grateful that you caught it, since I never would have seen it in my normal usage. I just pushed a change to the develop branch with a client-specific implementation, and it seems to properly contain the plugin's behavior to tsserver buffers. Could you take a look at the implementation and let me know your thoughts?

[folke]

Changes look great!

One suggestion:

Maybe instead of needing to do

client.request = ts_utils.create_request_handler(vim.deepcopy(client.request))```

Consider doing something like:

ts_utils.setup_client(client)

[folke]

Also, is the vim.deepcopy needed here?

[jose-elias-alvarez]

Wow, nice idea – really cleans up setup on the user side. You're also right that vim.deepcopy wasn't necessary – I thought it would avoid duplicating the original handler, but in fact, that issue had to be solved in a different way.

Let me know if you're able to try it out. I'm going to use it at work this week and then merge it if everything looks good so we can get some more eyes on it.

[folke]

@jose-elias-alvarez that endless loop was cause by my local changes btw 🤦‍♂️

on_attach is run for every new buffer, so the client.request_orig on my branch was replaced by my injection code the second time.

This is not a problem with your code, since you pass the original handler, but I believe handlers will be overridden multiple times for every new TS file you open. You might want to set a variable on the client, to be sure not to inject the request method more than once.

[jose-elias-alvarez]

That's exactly how I handled it! Hopefully this isolates everything properly.

[jose-elias-alvarez]

Just merged!