Closed premell closed 2 years ago
I'm not familiar at all with react, but I'll assume that useEffect
is a function defined in a module not imported by default, and when completing, choosing the plain-text useEffect()
also inserts a import aModuleContainingUseEffect
at the top of the file, which doesn't happen when useEffect(${1:arg1}, ${2:arg2})
is chosen.
Are the completions coming from lsp or du you have a snippet defined for useEffect
?
I am not quite sure what you mean with plain-text. Do you mean the standard LSP completion? Because the LSP completion auto imports but the snippets in the form of useEffect(${1:arg1}, ${2:arg2})
does not. The snippet is coming from friendly snippets.
When using the LSP completion you can see it states auto import from react
in the top
This is the snippet from friendly snippets, and it does not auto import
It will be a pain to implement this on our side, maybe you will get better results by following this path: https://github.com/typescript-language-server/typescript-language-server/issues/235
hmm I am not sure how I can use the ts server for this, since ts server has nothing to do with snippets.
Considering the LSP suggestion has "Auto import from react" in the top, I'd assume that LSP has some api for imports. In that case, you could extend the snippet template with an optional field with the import, so that it doesnt break current snippets, also many snippets doesnt require imports.
The problem is obviously that you have to change the snippet template which would make the snippet engines incompatible with other snippet managers. It's always really difficult create a new standard like this, but I am unsure of how to add auto imports otherwise. Maybe some way of extending 3rd party snippets, but that would make them very hard to maintain
Well, we already support
luasnip
-field in the json (used currently for auto-triggered snippets)The only piece that's somewhat missing is inserting the import
-statement (preferably telling the lsp to auto-import it), but if we can figure that out, I'd have no objections to implementing it.
I think we can try adding some luasnip-specific stuff to friendly-snippets
via PR, if they don't like it, we can still fork
I tried out some stuff, seems like auto-imports are generally code actions and we'd have to query all code actions at some position in the code and then send back the one that adds the import-statement. Could be hard, especially if multiple imports are possible, we could open the nvim code action-menu (:lua vim.lsp.buf.code_action()
) so the correct one can be chosen.
I've tried to look at different code bases to see how they handle auto imports, but I am not familiar with LSP so its hard to follow. Btw what do you mean with querying all code actions?
"especially if multiple imports are possible". Cant you just make the import statements to an array in the snippet json, and loop over the array and applying the code action?
I still think that it's overcomplicating the implementation as that's something that lsp does for you, it's not good enough to give more priority to the lsp provider so the useEffect
from lsp appears first? Otherwise you will have to deal with the action that lsp server provides(if it does) cause lsp is "typing" the text not picking it from the menu.
@leiserfg but the lsp auto competition does not have snippets. So you either have to write the entire expression "useeffect(() => { ... },[])" or you have to manually import. Would be nice to have auto import together with the expression
I've tried to look at different code bases to see how they handle auto imports, but I am not familiar with LSP so its hard to follow. Btw what do you mean with querying all code actions?
Check here for the source of the code_action implementation for nvim, in the handler there is a loop going over the returned actions.
"especially if multiple imports are possible". Cant you just make the import statements to an array in the snippet json, and loop over the array and applying the code action?
No :/ There may be code actions that have nothing to do with importing, or multiple Modules that provide this function (Rusts' Result iirc), importing all of them wouldn't make sense
I think the best we can do is, if there's one, do that one, and if there are many, let the user decide
Going to close this, I don't think we'll add anything like this
I'm not yet 100% happy with it, but this is my current solution for an example of a React useEffect
. It automatically adds the missing import statement (or nothing it it is already there) and also automatically inserts the dependencies based on the hook you wrote.
The basic approach is simple: use node callbacks to trigger the LSP code action function and apply a filter with automatic execution (if only one matches). Unfortunately does the TypeScript language server not offer these code actions for range code actions. So these requests must be made at the correct position in the buffer. Hence why the useEffect
is an insert node that is intended to be jumped over only, just to trigger the code action. Same goes for the empty insert node for the dependency list you have to jump over.
In best case this snippet would have only a single insert node which is the callback function of the effect. But that does not work. That's because of the range/position issue.
For the dependency part, it is theoretically possible to manually execute "special" LSP commands. But somehow they are more tricky than expected to execute. Anyhow, this does not work for the import actions, as these use no commands but actual workspace edits. So I settled on the code action filter plus automatic execution approach.
--- @param title_pattern string
local function maybe_run_code_action_with_matching_title(title_pattern)
return function()
vim.lsp.buf.code_action({
filter = function(action)
return action.title:match(title_pattern) ~= nil
end,
apply = true,
})
end
end
return {
{
snippet(
'useEffect',
format(
[[
<effect>(() =>> {
<callback>
}, [<dependencies>])
]],
{
effect = insert_node(1, 'useEffect'),
callback = insert_node(2, '// TODO'),
dependencies = insert_node(3, ''),
}
),
{
callbacks = {
[1] = {
[snippet_events.leave] = maybe_run_code_action_with_matching_title(
'import from "react"'
),
},
[3] = {
[snippet_events.leave] = maybe_run_code_action_with_matching_title(
'Update.*react%-hooks%/exhaustive%-deps'
),
},
},
}
,)
},
Instead of using the simple vim.lsp.buf.code_action
method, it would be possible to do a manual LSP call. That would allow to set the position to an arbitrary location and not just the cursor position. The issue is how to get these positions. I know there is the Node:get_buf_position
method. But the issue is that it is not possible to use plain text nodes for these, because callbacks don't trigger for them.
@L3MON4D3 could you maybe help with some knowledge? In fact I wanna have something like a callback which gets triggered probably after the snippet is done? In best case also earlier, but text nodes have no index, so they get resolved immediately. Which is too early for the dependency code action at least.
PS:
@L3MON4D3 why do callbacks not work for multi_snippet
s? I tried to put them into the opts
as well as common
properties. But they don't trigger.
@L3MON4D3 Would it be a valid feature request to define callbacks directly at their nodes? Like avoiding the indexing issue and have it within the general Node options?
Like an example:
snippet(
'foo',
format('foo {bar}', {
bar = text_node('bar', {
callbacks = {
[snippet_events.enter] = -- ...
})
}
)
Note that this even enables callbacks for text nodes đŦ
Right now I try to figure out if I can use a [-1]
callback to access all nodes in the snippet. So I could text nodes instead of insert nodes and get their buffer positions and trigger the code action request with a custom position. The node
(snippet) argument to the callbacks is just huge. Trying to read the source code see if there is anything that allows me to access the nodes.
Trigger alert: ugly prototyping code ahead!
Here is my solution using callbacks for the overall snippet on enter and leave and remove the unnecessary insert nodes with plain text nodes that are addressable via their keys. This does exactly what it should. It might not easily work for all cases. And it is still quite some boilerplate code. I'll try to refactor my code.
(code was adapted in the course of this ticket to improve behavior)
A module somewhere:
local code_action_method_name = require('vim.lsp.protocol').Methods.textDocument_codeAction
--- @param line number
--- @param character number
--- @return table
local function build_code_action_request_parameter(line, character)
local textDocument = vim.lsp.util.make_text_document_params(0)
local position = { line = line, character = character } -- TODO: Mind language server offset encoding.
local range = { start = position, ['end'] = position }
local diagnostics = vim.lsp.diagnostic.get_line_diagnostics(0, line)
local context = { diagnostics = diagnostics }
return { textDocument = textDocument, range = range, context = context }
end
--- @param line number
--- @param character number
--- @return table<table>
local function get_code_actions_for_position(line, character)
local parameter = build_code_action_request_parameter(line, character)
local server_responses = vim.lsp.buf_request_sync(0, code_action_method_name, parameter)
local code_actions = {}
for _, response in ipairs(server_responses) do
vim.list_extend(code_actions, response.result or {})
end
return code_actions
end
--- @param title_pattern string
--- @return table | nil
local function find_matching_action(all_code_actions, title_pattern)
return vim.tbl_filter(function(code_action)
return code_action.title:match(title_pattern) ~= nil
end, all_code_actions)[1]
end
--- @param code_action table | nil
--- @return nil
local function execute_code_action(code_action)
if code_action ~= nil then
if code_action.command ~= nil then
vim.lsp.buf.execute_command(code_action.command)
elseif code_action.edit ~= nil then
vim.lsp.util.apply_workspace_edit(code_action.edit)
end
end
end
--- @param node_snippet table
--- @param title_pattern string
--- @param node_key string
--- @return nil
local function maybe_run_code_action_with_matching_title_at_nodes_position(
node_snippet,
title_pattern,
node_key
)
local node = node_snippet:get_keyed_node(node_key)
local start_position, _ = node:get_buf_position()
local line, character = start_position[1], start_position[2]
local all_code_actions = get_code_actions_for_position(line, character)
local matching_code_action = find_matching_action(all_code_actions, title_pattern)
execute_code_action(matching_code_action)
end
--- @param title_pattern string used to filter code actions for, only if a
--- single code action matches it will b executed
--- @param node_key string key option of the node at which buffer location to
--- trigger the code action request
--- @param server_delay number | nil milliseconds to wait before query language
--- server, adapt to let slow servers sync with
--- buffer changes of snippet (default 200)
--- @return function callback function that can be registered for a snippet
local function get_lsp_code_action_callback(title_pattern, node_key, server_delay)
server_delay = server_delay or 200
return function(node_snippet)
vim.defer_fn(function()
maybe_run_code_action_with_matching_title_at_nodes_position(
node_snippet,
title_pattern,
node_key
)
end, server_delay)
end
end
return {
get_lsp_code_action_callback = get_lsp_code_action_callback,
}
Example snippet file:
-- Require statements for LuaSnip stuff (or not if you use a prelude)
-- ...
local get_lsp_code_action_callback = require('module.somewhere').get_lsp_code_action_callback
return nil,
{
snippet(
'useEffect',
format(
[[
<hook_name>(() =>> {
<callback>
}, [<dependencies>])
]],
{
hook_name = text_node('useEffect', { key = 'hook_name' }),
callback = insert_node(1, '// TODO'),
dependencies = text_node('', { key = 'dependencies' }),
}
),
{
callbacks = {
[-1] = {
[snippet_events.enter] = get_lsp_code_action_callback(
'import from "react"',
'hook_name'
),
[snippet_events.leave] = get_lsp_code_action_callback(
'Update.*react%-hooks%/exhaustive%-deps',
'dependencies'
),
},
},
}
),
}
Interactions via code actions seem to be unreliable. Sometimes they work like charm. But sometimes the language server seems to be not yet ready for the buffer change and does not provide the desired code action. đ¤ˇđž đđžââī¸
Oh this is really cool!! Nice work! Would you put it in the Wiki?? :))
PS: @L3MON4D3 why do callbacks not work for multi_snippets? I tried to put them into the opts as well as common properties. But they don't trigger.
Oh, they have to be in the key common_opts
:
ls.setup_snip_env()
ls.add_snippets("all", {
ms({"asdf", "qwer"}, {i(1, "zxcv")}, {common_opts = {callbacks = {[1] = {[events.enter] = function()
print("enter from ms!!")
end}}}})
})
This is to mirror the common
-option in the first table.
@L3MON4D3 Would it be a valid feature request to define callbacks directly at their nodes? Like avoiding the indexing issue and have it within the general Node options?
Certainly :D The reason for them being set through the snippet is purely historical IIRC, it was just more comfortable to tack the callbacks on in the snippet, than to enable per-node-options. But since we have those now, it should be no big issue to allow a better api for them :) (this is me speaking without having taken a closer look, there's a very slight chance I'm misremembering this)
Note that this even enables callbacks for text nodes đŦ
I think we can say something like "this only works for insertNode", should be fine :D I'm sure I can do a PR for this soon-ish
Small suggestion:
There actually is api for getting a node by its key, try key_node = snippet:get_keyed_node(key)
The sync-lsp-stuff seems pretty cool, Re. the unreliability, have you tried something like vim.defer_fn
?
Oh this is really cool!! Nice work! Would you put it in the Wiki?? :))
I might. I'm not yet really happy with it. And as I said, it doesn't work fully reliable yet. Plus, with the potential node local callbacks, this will much cleaner and simpler (to some degree).
I think we can say something like "this only works for insertNode", should be fine :D
But it would be especially great to have this also for other nodes. đ
Like in this specific example I use them explicitly to get their buffer positions etc. I used the key
property to make accessing them more convenient. But callbacks, directly assigned to a node would receive the node itself as first parameter, which resolves that linking issue.
Just the order/indexing is not trivial ofc. đ
I'm sure I can do a PR for this soon-ish
I'd be happy if you link it here or somewhere đ
There actually is api for getting a node by its key, try key_node = snippet:get_keyed_node(key)
Nice, thank you! That cleans the code up a little. Sorry, the node interface is huge. đ
The sync-lsp-stuff seems pretty cool, Re. the unreliability, have you tried something like vim.defer_fn?
Yes, I tried the defer part. Somehow it didn't work. So even with 5000
ms I did not notice any lagging or so. Have to investigate again. There might be also other mechanics to force a change tick or similar.
Was pretty ugly to debug. Because while posting it, it worked like charm and very reliably. New NeoVim instance with new spawned language servers and it became an issue. âšī¸
Similar with executing server specific commands. They heavily rely on buffer versions and change ticks.
I spent quite some time on this yesterday. Might gonna look a little more into this soon.
The callbacks on node option level would be a nice feature. Though, so far in my snippets I actually use it always on the snippet level ([-1]
) to clean up things or similar.
The callbacks also for text nodes or function nods would be great too. But that opens the issue with when they get executed. Would be kinda cool to have them in order as they appear in the snippet. But I think you have some fancy algorithm with special logic there already. Don't wanna mess with it. But if I do, in the above example I would like to have first the callbacks (enter then leave) be called for the first node which is a text node. Then the (here not used) callbacks for the second node which is an insert node (with jump index 1) and then the final text node. Ofc I don't count the text nodes created implicitly by the format
function. But they basically just don't have options defined. So should work fine. đ¤ˇđž
Current state:
After using the vim.defer_fn
properly ( đŦ ), the import part works reliable now for my setup. It is not nice to reproduce, but I got some certainty.
Just have to figure out now why the second part with the dependencies does no more work for me. đ¤
Afterwards I try to get some clean and nice to (re)use code. Potentially with some improvements of the plugin API as discussed above. đ
Found the issue. I got "inspired" by the implementation of the original vim.lsp.buf.code_action
function, which uses the vim.lsp.diagnostic.get_line_diagnostics
function when building the request parameters. But in the NeoVim docs, this function is marked as depricated and you should use vim.diagnostics.get
instead. But this function does not return the diagnostic in protocol format.
Turned out the language server needs these diagnostics to process it correctly. At least the ESLint server needs this. The TypeScript server is fine with it/ignores it. The TS server provides the import action, so that one worked just fine. The dependency action comes from the ESLint server.
I applied my fixes in the code above to avoid confusion etc.
Still trying to clean up code.
EDIT:
I'm kinda okay with the code now. It should be somewhat readable and "nice" to use. Having it in a separate file makes it re-usable and somewhat hidden. đ
Having callbacks in node options would remove the need for the key
option and thereby the need to retrieve the node by its key. It would then also split the callbacks properly for each node where a code action is of interest.
Ahhh, okay that sounds like a bit of a weakness of the lsp-api neovim has currently.. but honestly, this is pretty specific, so probably fine :D Glad that you figured it out :+1:
But it would be especially great to have this also for other nodes. đ Like in this specific example I use them explicitly to get their buffer positions etc. I used the key property to make accessing them more convenient. But callbacks, directly assigned to a node would receive the node itself as first parameter, which resolves that linking issue. Just the order/indexing is not trivial ofc. đ
Ah, okay mhhh.. I've opened #1092 with a quick&dirty implementation (though everything should work :D), let's discuss there
I have now a solution I'm quite happy with. It builds on top of the new node_callbacks
feature by #1092, though it also works without, just less convenient/flexible.
While I'm happy with the UX/DX and reliability in the LSP communication, I'm not happy wit the amount of code it is and the maintenance it (might will) require. Having the #1092, something similar to the short callback function from this comment would be easy enough. Unfortunately is the whole LSP thing not that trivial. Somehow NeoVim its native incremental change synchronization is not fast enough (even though it already flushes change events before making requests). Additionally some servers seem to depend on their own diagnostics which have to be published first before they can be included in the code action request.
Anyhow, I'm sharing my code here, though I recommend everyone to not simply copy paste it. Please check the conversation of this issue and the linked PR to get into the details. It is not great code, but I hope somewhat readable.
Staying with the ReactJS hooks API example from this ticket in advanced setup:
It would be great to have the same auto import functionality as you do with LSP expansions. For example in react, if I auto complete
useEffect
in LSP,useEffect
gets auto imported. But when I use the luasnip snippet foruseEffect
it doesnt auto import.Its probably a really hard feature to implement, and requires a lot of cooperation between you, the snippet engines and LSP. But I figured I'd make an feature request anyway since its a great feature, and because many people wonder about auto import, its good to have an issue regarding it. Just so people can see the progress towards auto import, or if its impossible, what the reasons are.