lib/neovim-plugin: Add lazyLoad option to plugins #1875

psfloyd · 2024-07-16T06:20:48Z

This PR is the second split of PR #1866, it works on a solution for the issue #421.
This PR depends on PR #1874.
All suggestions from the original PR are included.

Design description

The idea is to automatically create lazyLoad option for all plugins that are defined with mkNeovimPlugin.
lazyLoad is conformed by triggers (e.g. filetype, event, key, command) and actions (e.g. functions to run before/after loading the plugin). mkNeovimPlugin would also generate default values for some of these options (e.g. the setup).

Plugin maintainers can decide to not create the lazyLoad option, by setting allowLazyLoading to false when defining the plguin.

Plugin maintainers can provide sensible default values to lazyLoad when defining a plugin. For example, if defining telescope.nvim, set Telescope as a command trigger. Also defining a custom setup function if they are setting callSetup as false.

Finnally users can :

Enable lazy-loading (globally or on a per-plugin basis)
Add to these triggers (or override them with mkForce) on a per-plugin basis.
Override actions on a per-plugin basis.

Progress

Adding mkLazyLoadOption to generate lazyLoad options for plugins.
Conditionally enabling lazy or regular loading depending on user settings in mkNeovimPlugin.
Adding and implementing a lazy-loading mechanism using lz.n.
Adding and implementing a lazy-loading mechanism using lazy.nvim.
Assert that only one of the backends is enabled.
Conditionally enabling lazy or regular loading depending on user settings in mkVimPlugin.

MattSturgeon

Just my initial thoughts. Again, thanks for working on this!

plugins/languages/qmk.nix

lib/options.nix

lib/neovim-plugin.nix

lib/helpers.nix

psfloyd · 2024-07-17T03:55:23Z

To keep things organized i am going to wait for PR #1874 to be stabilised to keep working on this.
Also I updated the top comment with a design description, to review the general concept.

MattSturgeon · 2024-07-17T22:57:49Z

Add to these triggers (or override them with mkForce) on a per-plugin basis.

Probably better to have our defaults set with mkDefault, allowing users to override without needing to resort to mkForce.

That's only a guideline though, not a rule. There may be times where a default is so sensible that it makes sense not to use mkDefault?

arunoruto · 2024-08-08T12:34:47Z

Seems like #1874 got morgen! I am hyped for this feature 🙌🏻
I hope I can move from helix back to neovim when this is implemented!

psfloyd · 2024-08-08T23:50:54Z

Now that #1874 has been merged I am going to rework this PR to work with that implementation. While working on #1874 I discovered some flaws in the draft implementation of this PR, that need to be addressed.
Later today I am going to update the first comment to better reflect the tasks that need to be done.

I am also going to integrate with the new helpers implementation from #1948.

It also would be nice to integrate lazyLoad options with configLua from #1876, when/if that gets merged.

psfloyd · 2024-08-09T05:52:12Z

I refactored the lazyLoad options, decupling them from the specific lz-n backend.
The first commit 894e190 is just for testing purposes. It can be removed at the end.

MattSturgeon

I know this is still a draft, so some of my feedback may be irrelevant. But here's some early feedback in the hopes it helps.

Thank you for your continued efforts!

MattSturgeon · 2024-08-09T08:41:58Z

tests/test-derivation.nix

@@ -47,6 +48,8 @@ let
              dontRun ? false,
            }:
            lib.optionalString (!dontRun) ''
+              ln -s ${derivation} $out/${name}


I understand this is a temporary debugging change, but I think this highlights another reason we should probably move back to using linkFarm instead of a custom derivation, even if it's still grouped on a per-test-file basis.

#1989 does this, among other things.

We could of course add the package being tested to passthru manually, but linkFarm does that for us.

The old implementation also didn't work for this kind of test. Because the linkFarm was of all the tests.
With this change I can build a specific test and run that nixvim instance to test functionallity inside neovim.

Maybe I missed a better way to achieve this...

With this change I can build a specific test and run that nixvim instance to test functionality inside neovim.

I think what we want here is to be able to build a test's passthru to debug it. If the test fails, that's because it can't be built.

If I understand passthru correctly, it would allow us to bypass building the test derivation and instead use the derivation that was being tested.

In the case of a linkFarm we'd probably be building a passthru of a passthru, russian doll style.

This is something we ought to tackle in another PR, especially as our test infrastructure is somewhat in flux currently: #2015 #1989 #1986 etc

tests/test-sources/modules/lazy-load.nix

lib/options.nix

psfloyd · 2024-08-14T04:11:36Z

I just pushed an implementation for lazy loading with lz-n.
Because of the complexity and amount of lines, would it be better to split lazy loading implementations into a new file? Perhaps lib/lazy-load? What do you think?

MattSturgeon

Minor suggestions and attempts to understand the changes 🙂

MattSturgeon · 2024-08-14T05:14:09Z

lib/neovim-plugin.nix

+            (lib.removeAttrs (extraConfig cfg) [
+              "extraConfigLua"
+              "extraConfigLuaPre"
+            ])


This isn't a robust solution, we really need something like #1876 so that we can assign plugin-config to an option and then decide here how to use that option.

The problem is we're assuming extraConfig returns a "plain" attrset, instead of something like a mkIf, mkOverride, mkMerge, etc.

It's also kinda "pulling the rug out" from the module definition, which is expecting a config definition to actually add a definition to the respective option.

If we have an intermediate, plugin-specific option, this is more transparent and also exposes better hooks to end-users.

MattSturgeon · 2024-08-14T05:22:07Z

lib/neovim-plugin.nix

+                    if cfg.lazyLoad.after == null then
+                      mkFn (
+                        (extraConfig cfg).extraConfigLuaPre or ""
+                        + optionalString callSetup ''require('${luaName}')${setup}(${
+                          optionalString (cfg ? settings) (helpers.toLuaObject cfg.settings)
+                        }) ''
+                        + (extraConfig cfg).extraConfigLua or ""
+                      )


Could this be the after option's default, instead of null? This if then else is effectively setting a default.

Don't mind too much if it is set with default in mkOption, or if we use lib.mkDefault in a config definition. We should probably also have a defaultText = literalExpression "" too, for transparency.

I guess this is more complex because mkLazyLoadOption isn't given an actual option path, so it can't (truly) know which extraConfigLua definitions are being used, etc. Same for luaName, setup, etc.

We can still implement it as a ${namespace}.${name}.lazyload.after = mkDefault (mkFn ""), we just would struggle to document this in defaultText

MattSturgeon · 2024-08-14T05:24:31Z

lib/options.nix

+      originalName,
+      lazyLoadDefaults ? { },


Suggested change

originalName,

lazyLoadDefaults ? { },

pluginName,

defaults ? { },

MattSturgeon · 2024-08-14T05:36:47Z

tests/test-derivation.nix

@@ -47,6 +48,8 @@ let
              dontRun ? false,
            }:
            lib.optionalString (!dontRun) ''
+              ln -s ${derivation} $out/${name}


With this change I can build a specific test and run that nixvim instance to test functionality inside neovim.

I think what we want here is to be able to build a test's passthru to debug it. If the test fails, that's because it can't be built.

If I understand passthru correctly, it would allow us to bypass building the test derivation and instead use the derivation that was being tested.

In the case of a linkFarm we'd probably be building a passthru of a passthru, russian doll style.

This is something we ought to tackle in another PR, especially as our test infrastructure is somewhat in flux currently: #2015 #1989 #1986 etc

MattSturgeon · 2024-08-14T05:37:36Z

lib/neovim-plugin.nix

@@ -43,9 +43,14 @@ with lib;
      extraPackages ? [ ],
      callSetup ? true,
      installPackage ? true,
+      # lazyLoad
+      allowLazyLoad ? true,
+      packageName ? originalName, # Name of the package folder created in {runtimepath}/pack/start or {runtimepath}/pack/opt


Is this not the same as luaName?

MattSturgeon · 2024-08-14T05:41:49Z

lib/neovim-plugin.nix

+          optionalPlugin =
+            x:
+            if isColorscheme then x else ((if x ? plugin then x else { plugin = x; }) // { optional = true; });
+          mkFn = str: if str != "" then "function()\n" + str + "end" else null;


mkFn feels like an over-abstraction

MattSturgeon · 2024-08-14T05:47:14Z

lib/neovim-plugin.nix

+          # Lazy loading with lz-n
+          (mkIf (lazyLoaded && config.plugins.lz-n.enable) (mkMerge [


I don't think it actually matters whether lz-n is enabled; we can define the config either way, it'll just have no effect, right?

We should probably assert somewhere that a supported lazy loading backend is enabled if the user has enabled lazyloading globally though.

MattSturgeon · 2024-08-14T06:41:02Z

Because of the complexity and amount of lines, would it be better to split lazy loading implementations into a new file? Perhaps lib/lazy-load? What do you think?

I'm not sure. The stuff in lib/options.nix could be in its own file if you prefer.

I don't think the stuff in mkNeovimPlugin can be moved without major changes?

MattSturgeon mentioned this pull request Jul 16, 2024

plugins/lz-n: init #1874

Merged

MattSturgeon reviewed Jul 16, 2024

View reviewed changes

psfloyd force-pushed the lazyLoad branch from 3fc3464 to c971cce Compare July 16, 2024 12:54

psfloyd force-pushed the lazyLoad branch from c971cce to 1f96fda Compare August 9, 2024 05:43

MattSturgeon reviewed Aug 9, 2024

View reviewed changes

psfloyd added 5 commits August 13, 2024 22:32

test/test-derivation: symlink test configurations on output directory

11c2371

lib/options: added mkLazyLoadOption

71939eb

lib/options: mkLazyLoadOption fixed defaults, arguments and paragraphs

106cf85

lib/neovim-plugin: lazyLoad lz-n backend

e325dd2

tests/lazy-load: added test

8cebb3f

psfloyd force-pushed the lazyLoad branch from 1f96fda to 8cebb3f Compare August 14, 2024 04:06

MattSturgeon reviewed Aug 14, 2024

View reviewed changes

MattSturgeon mentioned this pull request Aug 21, 2024

tests: move derivation to module-option & minor cleanup #1989

Merged

5 tasks

traxys mentioned this pull request Sep 9, 2024

[Feature] Lazy loading #421

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/neovim-plugin: Add lazyLoad option to plugins #1875

lib/neovim-plugin: Add lazyLoad option to plugins #1875

psfloyd commented Jul 16, 2024 •

edited

Loading

MattSturgeon left a comment

psfloyd commented Jul 17, 2024

MattSturgeon commented Jul 17, 2024

arunoruto commented Aug 8, 2024

psfloyd commented Aug 8, 2024 •

edited

Loading

psfloyd commented Aug 9, 2024

MattSturgeon left a comment

MattSturgeon Aug 9, 2024

psfloyd Aug 14, 2024

MattSturgeon Aug 14, 2024

psfloyd commented Aug 14, 2024

MattSturgeon left a comment

MattSturgeon Aug 14, 2024

MattSturgeon Aug 14, 2024

MattSturgeon Aug 14, 2024

MattSturgeon Aug 14, 2024

MattSturgeon Aug 14, 2024

MattSturgeon Aug 14, 2024

MattSturgeon Aug 14, 2024

MattSturgeon Aug 14, 2024

MattSturgeon commented Aug 14, 2024

		# Lazy loading with lz-n
		(mkIf (lazyLoaded && config.plugins.lz-n.enable) (mkMerge [

lib/neovim-plugin: Add lazyLoad option to plugins #1875

Are you sure you want to change the base?

lib/neovim-plugin: Add lazyLoad option to plugins #1875

Conversation

psfloyd commented Jul 16, 2024 • edited Loading

Design description

Progress

MattSturgeon left a comment

Choose a reason for hiding this comment

psfloyd commented Jul 17, 2024

MattSturgeon commented Jul 17, 2024

arunoruto commented Aug 8, 2024

psfloyd commented Aug 8, 2024 • edited Loading

psfloyd commented Aug 9, 2024

MattSturgeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psfloyd commented Aug 14, 2024

MattSturgeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattSturgeon commented Aug 14, 2024

psfloyd commented Jul 16, 2024 •

edited

Loading

psfloyd commented Aug 8, 2024 •

edited

Loading