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

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/helpers.nix
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ let
defaultNullOpts
mkCompositeOption
mkCompositeOption'
mkLazyLoadOption
mkNullOrLua
mkNullOrLua'
mkNullOrLuaFn
Expand Down
92 changes: 81 additions & 11 deletions lib/neovim-plugin.nix
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not the same as luaName?

lazyLoadDefaults ? { },
}:
let
namespace = if isColorscheme then "colorschemes" else "plugins";
cfg = config.${namespace}.${name};
in
{
meta = {
Expand Down Expand Up @@ -94,25 +99,90 @@ with lib;
example = settingsExample;
};
}
// optionalAttrs allowLazyLoad {
lazyLoad = helpers.mkLazyLoadOption {
inherit originalName;
lazyLoadDefaults =
(optionalAttrs (isColorscheme && colorscheme != null) { inherit colorscheme; }) // lazyLoadDefaults;
};
}
// extraOptions;

config =
let
cfg = config.${namespace}.${name};
extraConfigNamespace = if isColorscheme then "extraConfigLuaPre" else "extraConfigLua";
lazyLoaded = cfg.lazyLoad.enable or false;

# lz-n lazyLoad backend
# Transform plugin into attrset and set optional to true
# See `tests/test-sources/plugins/pluginmanagers/lz-n.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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mkFn feels like an over-abstraction

in
mkIf cfg.enable (mkMerge [
{
extraPlugins = (optional installPackage cfg.package) ++ extraPlugins;
inherit extraPackages;
}
(optionalAttrs callSetup {
${extraConfigNamespace} = ''
require('${luaName}')${setup}(${optionalString (cfg ? settings) (helpers.toLuaObject cfg.settings)})
'';
})
# Always set
(optionalAttrs (isColorscheme && (colorscheme != null)) { colorscheme = mkDefault colorscheme; })
(extraConfig cfg)

# Normal loading
(mkIf (!lazyLoaded) (mkMerge [
(extraConfig cfg)
{
extraPlugins = (optional installPackage cfg.package) ++ extraPlugins;
inherit extraPackages;
}
(optionalAttrs callSetup {
${extraConfigNamespace} = ''
require('${luaName}')${setup}(${optionalString (cfg ? settings) (helpers.toLuaObject cfg.settings)})
'';
})
]))

# Lazy loading with lz-n
(mkIf (lazyLoaded && config.plugins.lz-n.enable) (mkMerge [
Comment on lines +142 to +143
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(lib.removeAttrs (extraConfig cfg) [
"extraConfigLua"
"extraConfigLuaPre"
])
Comment on lines +144 to +147
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
extraPlugins = (optional installPackage (optionalPlugin cfg.package)) ++ extraPlugins;
inherit extraPackages;
}
{
plugins.lz-n.plugins = [
{
__unkeyed-1 = packageName;
after =
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 ""
)
Comment on lines +157 to +164
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else
cfg.lazyLoad.after;
inherit (cfg.lazyLoad)
enabled
priority
before
beforeAll
# after defined above
event
cmd
ft
keys
colorscheme
extraSettings
;
}
];
}
]))

# Lazy loading with lazy.nvim
]);
};
}
105 changes: 105 additions & 0 deletions lib/options.nix
Original file line number Diff line number Diff line change
Expand Up @@ -366,4 +366,109 @@ rec {
else
example;
};

mkLazyLoadOption =
{
originalName,
lazyLoadDefaults ? { },
Comment on lines +372 to +373
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
originalName,
lazyLoadDefaults ? { },
pluginName,
defaults ? { },

}:
mkOption {
description = ''
Lazy-load settings for ${originalName}.
'';
type =
with helpers.nixvimTypes;
let
triggerType = oneOf [
rawLua
str
(listOf str)
];
in
submodule {
options = with defaultNullOpts; {

enable = mkEnableOption ''
lazy-loading for ${originalName}
'';

# Spec loading:
enabled = mkStrLuaFnOr bool (lazyLoadDefaults.enabledInSpec or null) ''
When false, or if the function returns false, then ${originalName} will not be included in the spec.

Equivalence: lz.n => enabled; lazy.nvim => enabled
'';

priority = mkNullable number (lazyLoadDefaults.priority or null) ''
Only useful for start plugins (not lazy-loaded) to force loading certain plugins first.

Equivalence: lz.n => priority; lazy.nvim => priority
'';

# Spec setup
# Actions
beforeAll = mkLuaFn (lazyLoadDefaults.beforeAll or null) ''
Always executed before any plugins are loaded.

Equivalence: lz.n => beforeAll; lazy.nvim => init
'';

before = mkLuaFn (lazyLoadDefaults.before or null) ''
Executed before ${originalName} is loaded.

Equivalence: lz.n => before; lazy.nvim => None
'';

after = mkLuaFn (lazyLoadDefaults.after or null) ''
Executed after ${originalName} is loaded.

Equivalence: lz.n => after; lazy.nvim => config
'';

# Triggers
event = mkNullable triggerType (lazyLoadDefaults.event or null) ''
Lazy-load on event. Events can be specified as `BufEnter` or with a pattern like `BufEnter *.lua`

Equivalence: lz.n => event; lazy.nvim => event
'';

cmd = mkNullable triggerType (lazyLoadDefaults.cmd or null) ''
Lazy-load on command.

Equivalence: lz.n => cmd; lazy.nvim => cmd
'';

ft = mkNullable triggerType (lazyLoadDefaults.ft or null) ''
Lazy-load on filetype.

Equivalence: lz.n => ft; lazy.nvim => ft
'';

keys = mkNullable (listOf helpers.keymaps.mapOptionSubmodule) (lazyLoadDefaults.keys or null) ''
Lazy-load on key mapping. Use the same format as `config.keymaps`.

Equivalence: lz.n => keys; lazy.nvim => keys
'';

colorscheme = mkNullable triggerType (lazyLoadDefaults.colorscheme or null) ''
Lazy-load on colorscheme.

Equivalence: lz.n => colorscheme; lazy.nvim => None
'';

extraSettings = mkSettingsOption {
description = ''
Extra settings to pass to the lazy loader backend.
'';
example = {
dependencies = {
__unkeyed-1 = "nvim-lua/plenary.nvim";
lazy = true;
};
};
};
};
};
default = lazyLoadDefaults;
};
}
8 changes: 3 additions & 5 deletions tests/test-derivation.nix
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ let
buildPhase = lib.optionalString (!dontRun) (
''
mkdir -p .cache/nvim
mkdir $out
''
+ lib.concatStringsSep "\n" (
builtins.map (
Expand All @@ -47,6 +48,8 @@ let
dontRun ? false,
}:
lib.optionalString (!dontRun) ''
ln -s ${derivation} $out/${name}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


echo "Running test for ${name}"

output=$(HOME=$(realpath .) ${lib.getExe derivation} -mn --headless "+q" 2>&1 >/dev/null)
Expand All @@ -58,11 +61,6 @@ let
) testList
)
);

# If we don't do this nix is not happy
installPhase = ''
mkdir $out
'';
};

# Create a nix derivation from a nixvim configuration.
Expand Down
39 changes: 39 additions & 0 deletions tests/test-sources/modules/lazy-load.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
example = {
plugins.lz-n.enable = true;
plugins = {
cmp = {
enable = true;
settings = {
sources = [ { name = "snippets"; } ];
mapping.__raw = "require('cmp').mapping.preset.insert()";
};
};

nvim-snippets = {
enable = true;
lazyLoad = {
enable = true;
event = "InsertEnter";
};
};

telescope = {
enable = true;
lazyLoad = {
enable = true;
cmd = "Telescope";
};
};
};

colorschemes.ayu = {
enable = true;
settings.mirage = true;
lazyLoad = {
enable = true;
event = "InsertEnter";
};
};
};
}