Use libuv-file-watcher to update loaded snippet-collections. #1033

L3MON4D3 · 2023-10-06T10:37:51Z

Currently we register an autocommands on BufWritePost for all files present when load is called. This has two undesirable side-effects:

files that are added after the initial load are not loaded
files are only reloaded by the neovim-instance that edited them

This PR is mainly for swapping the autocommand-mechanism for a libuv-based one, but will also include some refactorings of the loaders in general, for example the cache, which is a bit hard to grasp.
One disadvantage of the libuv-file-watcher is that it does not work for all directories, for example if a file on a nfs-share is edited by one machine, a different machine will not receive events for that edit (at least this is the case for my setup). This means we should probably have some kind of per-collection-fallback (maybe even back to autocommands), and/or a manual command for re-checking (which could also be called periodically..?)

One point I'm not yet sure about is whether snippets whose origin-file is removed should also be removed?

As of now, the mechanism works for the lua-loader, since it's the easiest one :D

bew · 2023-10-08T13:36:29Z

Looks like it's working for the most common case now 👍

I might have an edge one: I use a _utils.lua file in the same directory as the snippets, with a number of helpers.
When I edit something in there / add a new helper, it's not available in the snippet files. Is that normal?

L3MON4D3 · 2023-10-08T13:44:30Z

Ah, that's an interesting case.
I think the easiest way to get that to reload would be using loadfile with the absolute path, since require (I assume you're using that right now?) caches the initial load forever, whereas loadfile (in neovim) uses a cache that updates on file-changes (so quick load on no changes, reload on changes)

Did this work correctly before? I'd be surprised if it did, but if it did, we'd have to replicate that :|

I'll assume you're by far not the only person to do something like this, so that's definitely something to add to the documentation

bew · 2023-10-08T13:49:49Z

loadfile seems to work yes 👍
But it only works when re-saving a snippet file, only editing the utils file wouldn't propagate to reload the files using it.
Might be too complex to do though, for little benefit.
Once we can cancel the cache we could just document that each snippet file needs to be re-saved to be reloaded?

L3MON4D3 · 2023-10-08T15:01:55Z

Okay so we should be able to do this by substituting our own loadfile into snippet-files, and then build some kind of dependency-tree for the files.
Not sure if that won't end up being way overengineered though :D

L3MON4D3 · 2023-11-18T21:07:26Z

Alright, pretty sure this is done, I'm happy with tests, doc, etc.
@leiserfg (and others, ofc :D) Wanna take a cursory look? Or a closer look, if you have the time to spare? 😅

lua/luasnip/session/snippet_collection/init.lua

plugin/luasnip.lua

bew · 2023-11-19T00:35:40Z

With a quick look around, I'm surprised by the amount of new code in each from_xxx loaders.
And I see each have its own Collection object:

LuaSnip/lua/luasnip/loaders/from_lua.lua

Line 188 in 6e1523f

local Collection = {}
LuaSnip/lua/luasnip/loaders/from_snipmate.lua

Line 137 in 6e1523f

local Collection = {}
LuaSnip/lua/luasnip/loaders/from_vscode.lua

Line 267 in 6e1523f

local Collection = {}

Isn't there a way for you to refactor this Collection type to avoid duplication, and only have loader-specific behavior as dedicated impl?
What's different between each Collection anyway?

L3MON4D3 · 2023-11-19T07:58:39Z

Isn't there a way for you to refactor this Collection type to avoid duplication, and only have loader-specific behavior as dedicated impl? What's different between each Collection anyway?

So, the first big difference between the (lua and snipmate) and the vscode-loader is that the vscode-collection declares all files that belong to it, and to what filetype, in a manifest (package.json), while the other two consist of a directory-tree, and the filenames determine the filetype.
Putting those two into one seems very impractical.

There's more potential with the lua and snipmate-loader, but those also have some differences, for example the handling of extends in snipmate needs quite a bit of code, while the lua-loader has to handle the ls_tracked_dofile-dependencies.
What is very similar in those two are add_file, reload and stop, but those methods don't amount to much compared to eg. load_file, or new, so merging them is probably not worth it.

L3MON4D3 · 2023-11-19T07:59:09Z

(What happened to your comment on the unquoted paths? Are they not actually an issue?)

This is necessary for the key-invalidation in add_snippets, which expects that retrieve_all always returns the same objects.

calling add_edge twice with the same vertices will not connect the vertices twice.

Since these functions are called by eg. all the loaders, this removes some potential for cyclic dependencies. Also add enqueable-operations-wrapper around refresh_notify and clean_invalidated, to prevent sending multiple updates for the same filetype in the same "tick"(?), and to remove some overhead that would result from calling clean_invalidated in quick succession (user doesn't care if snippets are removed a few milliseconds later, ofc).

If the same snippet-object is added to multiple filetypes, only the first filetype receives the source-information. This is actually done by the vscode-package-loader, so not a theoretical concern, I guess jump-to-snippet is just not used enough for this to get noticed.

Previously, we could not * add files that were not present when `load/lazy_load` was called to the collection. This is pretty annoying if one wants to add project-local snippets, or snippets for a new filetype (ofc). * load collections whose directory/package.json(c) did not exist when `load` was called. This is also an annoyance when creating project-local snippets, since a re-`load()` is required for the snippets to be picked up. * pick up on changes to the snippet-files from another neovim-instance (due to reloading on BufWritePost) This patch fixes all of these by modularizing the loaders a bit more, into one component ("Collection") which takes care of all the logic of loading different files, and another ("fswatchers") which notify the collections when a file-change is detected. This allows, first of all, a better design where the first concern can be nullified, and secondly, us to use libuvs api for file-watching, to implement the last two (if a potentially non-existing collection should be loaded, we can use libuv to wait for the collection-root/manifest-file, and create the collection once that exists). Another cool addition is the loader-snippet-cache, which makes it so that the snippet files (for vscode and snipmate) are only loaded once for all filetypes, and not once for each filetype. That's probably not noticeable though, except if a collection with many extends/languages for one json-file is loaded :D

L3MON4D3 linked an issue Oct 6, 2023 that may be closed by this pull request

Auto-reload snippets even if it was edited in another nvim instance #1008

Closed

L3MON4D3 mentioned this pull request Oct 6, 2023

Auto-reload snippets even if it was edited in another nvim instance #1008

Closed