Compiled colorscheme doesn't adapt to plugin changes/updates?

[tmillr]

I'm not sure exactly what's going on (or the cause of this), but here's what happened. BTW I'm using Plug.

In my vimrc, I changed Plug 'projekt0n/github-nvim-theme' to Plug '~/code/github-nvim-theme' (i.e. I changed it to use my local fork that I'm currently developing on so that it'd be easier for me to observe and test changes that I make to the plugin's source code). I then restarted Neovim and realized that many, if not all, of the changes I had made in my fork weren't present or taking effect. To fix the issue, I had to manually run rm ~/.cache/nvim/github-theme/github* and then restart Neovim again.

Also, FWIW, Neovim introduced a new Lua loader in v0.9.0 which shims loadfile() to compile and cache any Lua file that gets loaded in Neovim (including, presumably, any required file since loadfile() probably underpins require()). However, it is documented as experimental atm and is only available from v0.9.0 onwards I believe. Perhaps it is worth looking-into instead? It would cut-down on repo/plugin size, number of tests, and maintenance overhead if this new feature covers the our use-case(s). It would be on the user to enable this themselves in their vimrc. If I'm not mistaken, this new feature should be able to accomplish more or less the same thing as the current implementation that is bundled within this plugin does. See :help lua-loader for details.

Note: I think this issue also keeps the GithubThemeInteractive cmd from working properly

[nullchilly]

To fix the issue, I had to manually run rm ~/.cache/nvim/github-theme/github* and then restart Neovim again.

We only recompile if user's config changed or there was some changes in github-nvim-theme/.git folder. However, we can add something similar to catppuccin/init.lua#L179-L186 to be able to do :GithubThemeCompile without having to restart neovim:

vim.api.nvim_create_user_command("GithubThemeCompile", function()
  for name, _ in pairs(package.loaded) do
    if name:match "^github-theme." then package.loaded[name] = nil end
  end
  require("github-theme").compile()
  require("github-theme").load()
end, {})

We can also add an autocmd similar to catppuccin/nvim#L188-195 that recompile on save if the current buffer's path matches github-theme if vim.g.github_theme_debug was on

if vim.g.github_theme_debug then
  vim.api.nvim_create_autocmd("BufWritePost", {
    pattern = "*/github-theme/*",
    callback = function(opts)
      vim.schedule(function()
        require("github-theme").compile()
        require("github-theme").load()
      end)
    end
  })
end

Neovim introduced a new Lua loader in v0.9.0.

This is what compile feature initially behaved in nightfox.nvim, we relied on impatient.nvim (which is now known as :h lua-loader). However, it meant that the theme was compiled twice (One from the colorscheme and one from impatient) and increased the compile binary size so I decided to bundle it directly into catppuccin/nvim which later got added back into nightfox.

It would cut-down on repo/plugin size, number of tests, and maintenance overhead if this new feature covers the our use-case(s).

I don't think so, the implementation is like 6 lines longer?

local lines = {
  [[return string.dump(function()]],
  file_content,
  [[end]]
}
local f = loadstring(table.concat(lines, "\n")

[tmillr]

We only recompile if user's config changed or there was some changes in github-nvim-theme/.git folder. However, we can add something similar to catppuccin/init.lua#L179-L186 to be able to do :GithubThemeCompile without having to restart neovim

I see that. I don't think this will catch uncommitted changes however (nor work with git worktree, i.e. where .git is a regular file), but I guess for the majority of users who aren't going to hack on it themselves, this won't be an issue.

I believe there is already such a command provided by this repo/plugin. The thing is, I shouldn't have to manually run this command every time the plugin gets updated just for updates to take effect. Right now, at least in this repo/plugin, it seems that it doesn't work properly when I make a brand-new repo clone (which is indeed a changed git dir, no?) and completely restart Neovim. I'll investigate/test further when I get the chance, but I believe I found a bug in the implementation, and I can propose a fix soon.

We can also add an autocmd similar to catppuccin/nvim#L188-195 that recompile on save if the current buffer's path matches github-theme if vim.g.github_theme_debug was on

if vim.g.github_theme_debug then
  vim.api.nvim_create_autocmd("BufWritePost", {
    pattern = "*/github-theme/*",
    callback = function(opts)
      vim.schedule(function()
        require("github-theme").compile()
        require("github-theme").load()
      end)
    end
  })
end

Thanks. I understand. I believe this repo already has something similar to this as well.

Neovim introduced a new Lua loader in v0.9.0.

This is what compile feature initially behaved in nightfox.nvim, we relied on impatient.nvim (which is now known as :h lua-loader). However, it meant that the theme was compiled twice (One from the colorscheme and one from impatient) and increased the compile binary size so I decided to bundle it directly into catppuccin/nvim which later got added back into nightfox.

I don't think bundling it is going to avoid double compilation because you have no way to know, or control, whether the user is using impatient.nvim, or the new Lua-loader, or some other solution, anyway. If I'm not mistaken, you may even be compiling unnecessarily due to this fact, which might even worsen performance (and lead to additional unnecessary files on the fs).

I believe that, in the not-so-distant future (if not now already), the proper way to handle this will be Neovim's builtin compilation, and for several reasons. Firstly, the user gets to choose what they'd like to do and only has to choose one time in one central location (instead of trying to keep several different plugin configs in-sync with each other). Secondly, as is also the case when utilizing any other feature that's already built-in to the host environment, there'll be less code (duplication), simpler/cleaner codebases, fewer tests to write and maintain, and less maintenance overhead. I mean, just look at all the headaches that packer causes due to its custom compilation feature...I see notices/warnings on every other nvim plugin's README warning about it causing problems with their plugin, and now, it seems to be causing similar issues with this plugin too. I'm assuming that Neovim's loader won't have these issues, but even if it does, the user can easily disable it globally via one simple option in their vimrc instead of having to go through every single plugin config while keeping them all in-sync.

Furthermore, is it really that much more beneficial to have just a few or so plugins capable of this feature (compiling/caching byte-code) when the majority of plugins do not? Most users have many/multiple plugins, and most of them do not have custom compilation built-in. In practice, the performance benefits are probably going to be relatively small compared to the alternative.

It would cut-down on repo/plugin size, number of tests, and maintenance overhead if this new feature covers the our use-case(s).

I don't think so, the implementation is like 6 lines longer?

In this repo, the current implementation for this appears to be broken. That's why I created this issue, and hence the necessity for thorough/proper testing. I suggest considering Neovim's builtin compilation instead for this reason (among others).

FWIW, the compiler file/module in this repo is actually 106 lines, not 6, and that's not including the additional code that it takes to use it. But it's not just about the size of the extra code/file, it's also the additional: maintenance overhead (e.g. refactoring, or writing/maintaining tests to make sure everything is working properly, both on its own, and when integrated), complexity of the codebase, number of tests, etc. Re-implementing several things that the host environment already provides leads to unnecessary maintenance overhead and duplicated code (including "duplicated" tests as these things are probably already tested upstream). It's just not as simple or clean, and it can complicate the codebase unnecessarily. That's fine if it's done for performance reasons, but only if the performance benefits are substantial enough (and worth the additional overhead).

It's not just about the compiler feature either. I came across a few things in this repo that appeared to be re-implementing functionality that is already provided by Neovim or LuaJIT (e.g. bit operations, tables to string conversion, hashing, etc.), although maybe there is a reason for all of it that I am not aware of.

[nullchilly]

I believe there is already such a command provided by this repo/plugin.

I believe this repo already has something similar to this as well.

No, this repo is based on nightfox which I haven't ported these features to yet.

If I'm not mistaken, you may even be compiling unnecessarily due to this fact, which might even worsen performance

The compiled files don't have the *.lua patterns so I don't think vim loader will interfere (?)

I suggest considering Neovim's builtin compilation instead for this reason (among others).

These complications goes beyond just compiling to lua bytecode. It skips loading a bunch of modules like modules/ utils/ etc

That's fine if it's done for performance reasons, but only if the performance benefits are substantial enough (and worth the additional overhead).

In the past, catppuccin/nvim#94 was considered a slow colorscheme because of too many features. It was slower than even nvim-cmp and people didn't like that.

Which is why we added the compiler to make startuptime goes straight from 10ms to 0.5ms

re-implementing functionality that is already provided by Neovim or LuaJIT (e.g. bit operations, tables to string conversion, hashing, etc.)

Where is the hashing function in luajit?

Also it is an unordered table hashing which (to my knowledge) was never done before in a lua module. It was a result of hours of experimenting on my part.

I mean, just look at all the headaches that packer causes due to its custom compilation feature...I see notices/warnings on every other nvim plugin's README warning about it causing problems with their plugin, and now, it seems to be causing similar issues with this plugin too.

I think it's pretty stable now as there's no issue reported on github or discord for a while. It's a trade-off that @ful1e5 was willing to take between adding more features and performance.

[tmillr]

I believe there is already such a command provided by this repo/plugin.

I believe this repo already has something similar to this as well.

No, this repo is based on nightfox which I haven't ported these features to yet.

github-nvim-theme/plugin/github_theme.vim

Lines 4 to 5 in f09a14e

	command! GithubThemeCompile lua require('github-theme').compile()
	command! GithubThemeInteractive lua require('github-theme.interactive').attach()

It's right here and it is also documented.

[nullchilly]

That doesn't include the lua modules reloading -> compile -> load the theme again

vim.api.nvim_create_user_command("GithubThemeCompile", function()
  for name, _ in pairs(package.loaded) do
    if name:match "^github-theme." then package.loaded[name] = nil end
  end
  require("github-theme").compile()
  require("github-theme").load()
end, {})

[tmillr]

re-implementing functionality that is already provided by Neovim or LuaJIT (e.g. bit operations, tables to string conversion, hashing, etc.)

Where is the hashing function in luajit?

I don't believe luajit has one, however, both Vim and Neovim have a sha256 fn :help sha256().

Also it is an unodered table hashing which (to my knowledge) was never done before in a lua module. It was a result of hours of experimenting on my part.

I see. So maybe it is necessary then (as a dependency for the compile feature). I could be wrong, but I think this sort of order-resistant hashing could be accomplished just by deterministically converting the table to a string (e.g. with vim.inspect) and then just sorting the string's lines before hashing it with sha256(). There's many ways it could be done.

I mean, just look at all the headaches that packer causes due to its custom compilation feature...I see notices/warnings on every other nvim plugin's README warning about it causing problems with their plugin, and now, it seems to be causing similar issues with this plugin too.

I think it's pretty stable now as there's no issue reported on github or discord for a while. It's a trade-off that @ful1e5 was willing to take between adding more features and performance.

The compiler feature?

I see. Nightfox might actually not have the bug I've been mentioning, but this repo does. The issue is here:

local git_path = util.join_paths(debug.getinfo(1).source:sub(2, -23), ".git")

and has to do with the fact that github-theme is 13 characters, while nightfox is 9, and there is no .git dir in lua/. Basically, changes to the plugin are never detected and so updates don't take effect unless/until the user's config changes. However, I believe there's probably a better way (instead of just changing the -23 to -27) to manipulate paths than doing string.sub() and hoping that the filename doesn't change/move.

---

Are these colorschemes intended to be compatible with vim as well? Because I saw some references to vim.api and vim.o and I don't think those are available in standard vim.

[nullchilly]

I think this sort of order-resistant hashing could be accomplished just by deterministically converting the table to a string (e.g. with vim.inspect) and then just sorting the string's lines before hashing it with sha256(). There's many ways it could be done

These functions are very, very slow. vim.inspect - a debug function which also include sorting keys is very, very slow and sha256 is slower than djb2 by a huge margin. Furthermore we would need to use vim.json.encode to convert from table to string too. In fact, we used it in the past and it was even slower than loading the colorscheme itself. The current method is very optimized with the time of 0.009ms.

However, I believe there's probably a better way (instead of just changing the -23 to -27) to manipulate paths than doing string.sub() and hoping that the filename doesn't change/move.

This is just a fast and "reliable" way to get the repo's root. But yeah, PR this change.

Are these colorschemes intended to be compatible with vim as well? Because I saw some references to vim.api and vim.o and I don't think those are available in standard vim

Yes.

[tmillr]

Are these colorschemes intended to be compatible with vim as well? Because I saw some references to vim.api and vim.o and I don't think those are available in standard vim

Yes.

Hmmm it seems that polyfill is missing from this repo. I think @ful1e5 intentionally made this one nvim only.

[tmillr]

While #270 appears to have fixed the .git path bug (and I am no longer seeing a */lua/.git path appearing within my ~/.cache/nvim/github-theme/cache file), I'm still having stale cache issues where the plugin is not reacting to changes (e.g. I've tried both touching the .git dir after making uncommitted changes, and even completely switching branches). It's still broken. Just now confirmed again that the only way I'm able to get it to work is by doing something like rm -r ~/.cache/nvim/github-theme && nvim instead of just nvim, and therefore I am keeping this issue open and would like to hear from other users if they are experiencing a similar issue. And yes, I'm using a regular git repo with a regular .git dir inside (even though above I suggested making the compiler/cache compatible with .git files such as those used in/with git worktree).

[ful1e5]

@tmillr

Hmmm it seems that polyfill is missing from this repo. I think @ful1e5 intentionally made this one nvim only.

The reason Vim support is still included in the codebase is because it is built on Nightfox, which also has support for Vim and Neovim. During the refactoring process, I had to release the major version quickly as I had already delayed the release for too long. My intention was to have a strong foundation of theme structure to work with while retaining the previously supported features. Hence, I chose not to remove certain Vim implementations.

There are a few key reasons why I decided against providing full Vim support. First, Vim has its own distinct ecosystem and a range of plugins specifically designed for it. Including full Vim support would require extensive effort to ensure compatibility with these plugins. Additionally, Neovim and Vim are evolving in different directions, which could potentially result in complications in the future.

[tmillr]

After doing some debugging recently, I think I stumbled upon something which may be the cause for the stale-cache issues that I'm experiencing (although I'd need to investigate more to be certain). Either way it should be fixed.

The final hash is a direct string combination/concatenation of 2 parts, the hash of all settings (incl options, overrides, etc.) and then the literal mtime of the .git dir/file. Basically, when .git is not detected or doesn't exist (and it is impossible to derive the mtime part), the current impl falls-back to using the literal path to the .git dir/file instead as the mtime part. In theory this works nicely for nix setups where there is no .git dir and the absolute path to the project/repo dir will contain a unique hash based on the dir's contents. However, one issue with the current impl is that debug.getinfo().source can (and often does?) return a relative path which does not contain the aforementioned nix hash. The path used in this case often looks like ./.git.

What needs to be done is to change the current impl to convert the path from debug.getinfo().source to an absolute path (and resolve symlinks as well) before using it as part of the final hash. This won't add any noticeable overhead either, since vim.fn.getftime() is already doing this internally anyway (i.e. resolving symlinks).

#291

Edit: actually it seems that I am getting the full abs path when using nix. getinfo() probably automatically returns a shorter relative path when the path is within the current directory/PWD/CWD. This should still be accounted for IMO. It is also important for symlinks to be resolved as nix uses them often, and the unresolved paths can contain irrelevant hashes that can change even when the plugin hasn't.