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

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

Merged
merged 9 commits into from
Dec 2, 2023

Conversation

L3MON4D3
Copy link
Owner

@L3MON4D3 L3MON4D3 commented Oct 6, 2023

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

This comment was marked as resolved.

@L3MON4D3

This comment was marked as resolved.

@bew

This comment was marked as resolved.

@L3MON4D3 L3MON4D3 force-pushed the loader-luv branch 2 times, most recently from 5b2d9aa to 09a2959 Compare October 8, 2023 10:17
@L3MON4D3

This comment was marked as resolved.

@bew

This comment was marked as resolved.

@L3MON4D3

This comment was marked as resolved.

@bew

This comment was marked as resolved.

@L3MON4D3

This comment was marked as resolved.

@L3MON4D3

This comment was marked as resolved.

@bew

This comment was marked as resolved.

@bew

This comment was marked as resolved.

@L3MON4D3

This comment was marked as resolved.

@bew

This comment was marked as resolved.

@L3MON4D3

This comment was marked as resolved.

@bew
Copy link
Contributor

bew commented Oct 8, 2023

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
Copy link
Owner Author

L3MON4D3 commented Oct 8, 2023

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
Copy link
Contributor

bew commented Oct 8, 2023

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
Copy link
Owner Author

L3MON4D3 commented Oct 8, 2023

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 L3MON4D3 force-pushed the loader-luv branch 2 times, most recently from e175dab to 8e97f27 Compare October 10, 2023 14:08
@L3MON4D3 L3MON4D3 force-pushed the loader-luv branch 3 times, most recently from 7846069 to 4a92b51 Compare November 18, 2023 20:56
@L3MON4D3
Copy link
Owner Author

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? 😅

plugin/luasnip.lua Outdated Show resolved Hide resolved
@bew
Copy link
Contributor

bew commented Nov 19, 2023

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:

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 L3MON4D3 force-pushed the loader-luv branch 2 times, most recently from 1863177 to 29bc71f Compare November 19, 2023 07:52
@L3MON4D3
Copy link
Owner Author

L3MON4D3 commented Nov 19, 2023

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
Copy link
Owner Author

(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 L3MON4D3 force-pushed the loader-luv branch 2 times, most recently from 8ce5c37 to 3a47a1f Compare December 2, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants