doom-neovim / doom-nvim

A Neovim configuration for the advanced martian hacker
GNU General Public License v2.0
1.01k stars 107 forks source link

Feature Proposal: New module architecture #233

Closed connorgmeehan closed 2 years ago

connorgmeehan commented 2 years ago

I posted on the discord discussing ways for laying out a new, more modular architecture for modules in doom-nvim. This issue's goal is to propose how it might work and get feedback so we land on a good solution. Feel free to provide feedback to any part of this proposal. I will also use block quotes for areas that I'm looking for feedback.

This is just to get the discussion started, we should start on the implementation once doom-nvim 3.2 is released.

Overview

Each module in doom_modules.lua will have a config file determined by the module name i.e.: the lsp will have a doom-lsp.lua file. This file will be responsible for importing all of its dependencies via packer.use, binding all of its keymaps, adding all of its autocommands and adding any additional logic, commands and handlers.

I'm not sure if we should have a single file, such as doom-lsp.lua, or instead folders, doom-lsp/init.lua + doom-lsp/config.lua. I like the idea of having it all in one file but if a module contains a lot of logic it could make it harder to maintain. Having folders would also let us include a README.md for each module. Would both work?

Features and improvements

  1. Easier to find code relating to a specific feature
  2. Easier to manage modular architecture
  3. Per module keymaps, only binds what's necessary.
  4. Easier to configure LSPs, linters, documentation generators, code formatters per language
  5. Add custom logic, commands, keymaps for specific languages

Module API

Each module will return an object with certain properties, I think it's easiest to describe in lua.

local module = {}

-- USER SETTINGS
-- Settings related to a module (avoid changing down the line to reduce merge conflicts)
module.settings = {
  enable_some_functionality = false,
}

-- Custom configuration can be handled by a function 
local config = function()
  if settings.enable_some_functionality then
  end
  ...
end

-- IMPORTING DEPENDENCIES
-- I think it's best not to abstract libraries that people are used to, 
-- For this reason we just pass the packer use function into the module
module.packer = function(use)
  use({
    'nvim-treesitter/nvim-treesitter',
    config = config,
  })
end

-- BINDING KEYMAPS
-- Keymaps can be defined in an object, this should integrate with whichkey and nvim-mapper
-- Again, using familiar technologies feels like the best path to me and nest.nvim seems to be the most popular library to setup keymaps
-- We could also disable keymaps by creating new flags in doom_modules.lua such as +no_keymap
-- This could also be used to generate documentation or create a cheatsheet down the line
module.keymaps = {
  ...
}
return module

Are there any other features that should be standardized across modules?

Would we need a user_keymaps in case users wanted to add or edit existing keymaps (and would this be able to unbind the defaults?)

Language Module API

While some languages work great with the nvim-lspconfig defaults, others require a larger amount of customisability. For this reason I think languages should optionally have their own modules that fallback to sane defaults.

local module = {}

-- LSPS
-- Defines all the necessary LSPs for this language module
-- The key or ipairs value is used to automatically download the LSP via nvim-lspmanager
module.language_servers = {
  volar = {
    -- ... custom config to be passed into the language server
    -- This config 
  },
  'tailwindcss', -- LSP without custom config
}

-- LINTERS
-- Defines the linters for this language
module.linters = {
  'eslint',
}

-- DOCUMENTATION_GENERATORS
-- Documentation generator and configs possibly via neogen https://github.com/danymat/neogen
module.documentation_generators = {
  -- ...
}

-- FORMATTERS
-- Same as above
module.formatters = {
  'prettier',
}

-- Same behaviours as a regular module
local module.keymaps = {
  .. custom language feature
}

There are some instances where we need to configure multiple of the same LSP such as in the case of volar: https://github.com/johnsoncodehk/volar/discussions/606 How could we include this behaviour? One solution could be

module.language_servers = {
  -- language server name to install
  volar = {
    -- Name of 1st instance
    volar_api = {
      -- 1st instance config
    },
    -- Name of 2nd instance
    volar_doc = {
      -- 2nd instance config
    },
  }
}

Extra features and options

Doom config

We'll need a way of passing relevant doom_config options to these modules i.e. freeze_dependencies (disables pinned packer dependencies in /dev). This could be passed into the module.packer function:

module.packer = function(use, doom_config)
  if doom_config.freeze_depencies then
    ...
  end
end

We will also need a way to pass this into the config function of the packer dependency.

molleweide commented 2 years ago

Yo!! I have not read through your proposal in depth yet, but I want to add this reference here so that local plugin development is being accounted for so that toggling btw local and upstream plugins becomes as smooth as possible. https://github.com/NTBBloodbath/doom-nvim/issues/181

Now that I have gotten stuff to work again on both my computers, I am going to try to use Folkes local plugin dev pattern in doom and see how it feels goes because it should be pretty straight forward I think to test it.

connorgmeehan commented 2 years ago

Ok I've had a little read into this, can you provide a bit more information? So with this pattern have you replaced the references to the githup repositories in lua/doom/modules/init.lua with locally cloned versions? Or are you just adding to doom_plugins.lua with locally cloned versions.

molleweide commented 2 years ago

yo! I haven't tested his pattern yet. All I am saying is just that it is a bit of pain to develop plugins, with reloading and toggling between the live plugin and the local one so I think that this is something that should be accounted for. I have on my todo list to checkout a new dev branch and then copy the folke packer.util and see if it works out of the box on my computer.

I am also a beginner with plugins. I wrote my first plugin a couple of days ago and I have been testing how I can include the plugin and I have done it in all three of doom-plugins, command in doom config, and also straight into modules.init and it works but it is just not smooth when having to toggle btw live and local.

That is why I think it would be cool if you'd have somekind of tiny local plugins config table in doom_config where you could specify how this kind of thing should be handled.

molleweide commented 2 years ago

some of my testing has also been delayed because I was scarred to do stuff when tree-sitter was being an ass on my M1.

connorgmeehan commented 2 years ago

Yeah plugin development like this is a pain, I wouldn't even recommend importing a plugin that you're trying to develop into your main nvim config. Instead I'd run it in a seperate neovim instance.

Info on how to do that I've been working on a PR for nest.nvim [https://github.com/connorgmeehan/nest.nvim/tree/integrations-api](https://github.com/connorgmeehan/nest.nvim/tree/integrations-api) where I have a minimal vim config in the repo and a Makefile to start/test it. Once you've confirmed everything's working then that's when I'd import it and test it in doom-nvim, either using a local import or referencing your fork on github. This is how my directories are laid out: I have all of the plugin dependencies cloned inside of a folder and then I I can import the dependencies using `vim.api.nvim_command('set rtp+=../plenary.nvim')` ``` ./nvim_plugins ./nvim_plugins/nest.nvim ./nvim_plugins/which-key.nvim ./nvim_plugins/nvim-lua-plugin-template ./nvim_plugins/nvim-mapper ./nvim_plugins/nvim-lspmanager ./nvim_plugins/plenary.nvim ./nvim_plugins/nvim-treesitter ./nvim_plugins/telescope.nvim ``` Another option, would be testing your plugin within the doom-nvim-contrib worktree generated by the Dockerimage. If you put the plugin in `contribute/workspace` (generated after docker image with `./start_docker.sh`) you can add the following line to `doom_plugins`. ```lua { '~/workspace/' } Restarting `./start_docker.sh` should install the plugin and you'll be able to test it in the docker image.

That being said I think it's a good idea to be able to override dependencies with local/other forks. I'll make an feature request for this and aim to have it done for v3.3.

LuigiPiucco commented 2 years ago

I have read this thread and came up with a proposal on how we can make Doom Nvim modular and help it grow (plus I also hope to have solved the dependency override problem). I already began implementing my ideas, but I will take a moment to document them here before continuing. These ideas are a mashup between what's written here, Doom Emacs' architecture and LunarVim's also.

Preface

Despite the advertised intent of providing a Doom Emacs-like experience, when it comes to changing the defaults this project is still somewhat lacking, and its to be expected, given its age. In my opinion, we can get there faster if we actually use certain code and organization principles from Doom Emacs, which are currently ignored. As a long time user of the latter, I will begin by explaining how it works, and what can we mirror and what must we adapt.

Config files

Doom Emacs has 3 main config files in $DOOMDIR: init.el, config.el and package.el. All of these are just runnable elisp code that prepare the Emacs environment. None of them work by returning lists or tables, they are expected to be imperative and have side-effects, in the functional programming sense of these terms.

Translation to nvim

This proposal from the end-user's perspective

Files they'd need in our $DOOMDIR:

-- Here, `doom` aready has default values, no need to specify what's not overriden.

-- Some general doom options.
doom.freeze_dependencies = false
doom.autosave = true
doom.autosave_sessions = true
doom.swap_files = true
doom.backup = true
doom.preserve_edit_pos = true

-- Some module specific options.
doom.modules.dashboard.footer = "Rip and Tear"
doom.modules.treesitter.highlight.enable = false

-- This is a special option, its where you can put side effects for configuring anything
-- not handled natively by Doom. As an ephemeral example, which-key:
doom.post_startup = function()
  -- NOTE: When nest.nvim finishes implementing integration support, we could have a
  -- which key wrapper, but for now, this is is needed. 
  local wk = require("which-key")
  wk.register({
    P = { "<cmd>Telescope projects<CR>", "Projects" },
  }, { prefix = "<leader>" })
end

This is also where we'd have package overrides:

-- Here, we override nvim-tree.lua to come from my fork. Note that packages are group by module,
-- and the index for package `foo.nvim` of author `BarTolomeu` is just `foo.nvim`, but the key 1 of this
-- nested table should be "BarTolomeu/foo.nvim", this is passed verbatim to packer.
doom.packages.explorer["nvim-tree.lua"][1] = "BatTolomeu/nvim-tree.lua"
doom.packages.explorer["nvim-tree.lua"].commit = "SOMESHA";
doom.packages.explorer["nvim-tree.lua"].config = function()
  -- ...
end

Inside the doom.packages[module][package] is a table with the spec that's passed to packer. The only thing that you can't specify here is disable (because it will be overriden based on whether module is disabled or not). If you specify a config function, it won't replace the builtin one, but be run after it.

This proposal from the maintainer's perspective

The main thing here is that, instead of listing everything inside lua/doom/modules/init.lua, and having lua/doom/modules/config/doom-*.lua be the packer configs, we separate each module in a folder with path lua/doom/modules/<category>/<module>/. Inside this folder, there are two files: init.lua and packages.lua. packages.lua returns a table which contains keys to be placed in doom.packages[module] as the defaults, and on lua/doom/modules/init.lua we just merge all of these. init.lua returns a table with (currently) two keys: defaults, used to populate doom.modules[category][module], and packer_config, a table mapping package names to config functions that are then passed accordingly to packer.use. Also, it would be good to have READMEs in these folders to explain options available to each module.

What stage is this in

I'm currently porting module configs, the general framework is already pretty much done, but since there are quite a few of them and its a very tedious job, it may take a while.

Cons of this approach

connorgmeehan commented 2 years ago

Amazing! I love everything that's gone into this. I really like the imperative configuration. Especially grouping module related config to a table ('doom.modules.dashboard.footer = "Rip and Tear"'). I have also started implementing my version of this architecture but some of your decisions seem to make more sense. I'd love to work together on it. Are you on this discord yet? It could be a good place to talk about this.

Also, I briefly pushed my implementation which is focused on being as simple to implement as possible. You might be interested in seeing how I have been implementing it. Is your branch pushed anywhere?

https://github.com/NTBBloodbath/doom-nvim/compare/b5cc00cf9778%5E...4f5abe7645d4

molleweide commented 2 years ago

It's gonna be cool to follow this. Im gonna lear a lot

LuigiPiucco commented 2 years ago

Thanks for the positive feedback! It's currently not pushed anywhere, due to being incomplete. I could force push partial implementations for you to follow, though I will place a big NOT USABLE YET disclaimer for anyone that stumbles on it. Plus, expect half done commits to go missing, I like tidy my history before merge.

I have taken a glance (currently using my phone, sadly) at your version, Connor, there are some interesting pieces I took note of. In particular, there's already the which-key integration I was expecting to take longer to be possible, so I'll try joining it with mine.

About joining Discord, sure, I can once I go to the computer.

As a piece of discussion on my choices vs. yours, I see that your solution for breaking up package definitions per module is to set module.setup equal to a function that gets packer.use, whereas I wrap the definitions in a packages.lua and add the config and disable fields procedurally via is_plugin_disabled and a key in init.lua. I'm not sure one is better than the other. Do you have an opinion here? I guess having a table rather than direct packer usage makes overrides easier to implement, as you can change the package table prior to passing it to packer, rather than having to provide facilities to change the behavior of a function.

connorgmeehan commented 2 years ago

That's ok, my implementation has the same situation, last few commits have been WIPs but I'm rebasing to keep the history as clean as possible.
Yes so the whichkey integration has been done for a while now but I've been waiting on the nest.nvim maintainer to provide feedback and go through the approval process with him. I'll probably ping him in next year once things have quietened down.

I have a mild preference towards keeping the setup as a function as I think it's a bit more flexible, but in the end I don't really mind. My idea for overrides was the following:

-- In your `doom_startup.lua` file.
doom.plugin_overrides['plugin/name'] = {
    'different/source.nvim',
    config = function()
       -- custom config
    end
}

-- And then in the wrapped packer function.

  local wrap_packer = function(use)
    return function(spec)
      -- Disabled pinned commits if config option set
      spec.commit = pin_commit(spec.commit)

      -- Override config
      local plugin_name = spec[0]
      local plugin_override_spec = doom.plugin_overrides[plugin_name]
      local merged_spec = vim.tbl_extend('force', spec, plugin_override_spec or {})

      use(merged_spec)
    end
  end

This wouldn't be able to handle if two modules source the same plugin but I think from the user's perspective it's closer to the native packer.nvim api. Let me know what you think, happy either way :)

Edit: If you want an easier way to see what I've done I'm tracking changes of my implementation (may or may not even run) in this branch on my fork. I've just been trying to solve the issue of languages with edgecase LSP/treesitter configurations.

LuigiPiucco commented 2 years ago

I've finally pushed my changes here. I'll describe what I did and how it works, but I may not finish in one go, I'll edit this comment later if so.

In terms of user-facing files, here you can find my config, which is running and already usable, albeit without ported keybinds. What this config doesn't show is setting keybinds, and that's because I haven't implemented user binds, but that would be easy to do once I have time.

First, let's talk about internal stuff. The very first noticeable change is that doom_{modules,config,userplugins}.lua don't have the source field boilerplate. That's because I implemented explicit 'runtimepath' iteration to find the files, instead of relying on require. This sets the source of the objects returned from doing require("doom.core.config.{init,modules,userplugins}") automatically. See here for one of the implementations.

Next, I made doom.core.config (without .modules or .userplugins a bit special. It doesn't return what doom_config.lua returns, so you can no longer do require("doom.core.config").config.doom to get config values. This works very differently. The config module now has a load function that is called at the beginning of startup init.lua, and this function does two things. First, it populates a global doom with defaults to be used later, second it runs dofile on doom_config.lua, which is supposed to change this doom global to the user's liking. doom has the following structure:

The user can change and add to any of these, they will be iterated over and acted upon.

connorgmeehan commented 2 years ago

Amazing, I love the amount of thought and work that's gone into this, it's pretty incredible. I only have one reservation with this config and it's on cross module interactions.

Currently the treesitter module checks if the restclient is enabled and then handles that case inside of the treesitter config. Another interaction is that for the whichkey/mapper integrations, nest.nvim needs to re-run on the keybindings when whichkey/mapper has finished lazy loading. It's going to get pretty messy if we keep handling these cases in the parent module's config. Take a look at the config required for the best experience in volar.

I'd like to be able to extend doom-nvim's functionality and handle these edge cases in the child module's config.

Do you have an idea on how we could tackle this issue? I think it would also be good to be able to add this functionality without modifying the internals of doom-nvim. If there was a function that a module could call such as

use_module_field('keybindings', function(keybindings, category_name, module_name)
end)

This would pass the keybindings field from each module into the function.


I have some other smaller questions: Have you tested startup speeds and if so how does it perform? How much work will it take to ensure compatibility? Even if users have to make a small tweak, it would be good to ensure they don't have to rebuild their whole config.

-- Overall amazing job, keen to see what @NTBBloodbath thinks :)

LuigiPiucco commented 2 years ago

Currently the treesitter module checks if the restclient is enabled and then handles that case inside of the treesitter config.

I couldn't find this example in the required module, check if it hasn't been rebased, or give me a line reference.

It's going to get pretty messy if we keep handling these cases in the parent module's config.

I may have understood wrong, but in most cases I think we could, for example, check for treesitter inside restclient instead of for restclient in treesitter. It's mostly equivalent, though there's an exception: if the check happens to change something that is passed to setup, it must be in the parent module, because we can only call setup there. As a good example, I'm implementing which key binds by checking for each other module in the which key binds.lua, but I could very well check for which key in each module (none of this is pushed yet, though).

Another interaction is that for the whichkey/mapper integrations, nest.nvim needs to re-run on the keybindings when whichkey/mapper has finished lazy loading.

Unless I'm wrong about packer, nest.nvim is loaded and configured only once. The after key is there because we need which-key and mapper to be loaded before nest's config, since nest does require on them. Note that the which key config doesn't set binds, it just sets the package up. Binds are only set in the nest config, and propagated to which key via the integration.

Take a look at the config required for the best experience in volar.

I will, though I'm focusing on getting what was already working first before implementing new stuff. My current priority list is:

  1. Keybindings;
  2. Autocmds;
  3. Lsp corrections and optimizations (volar is here);
  4. Project.nvim module (probably for a separate PR).
use_module_field('keybindings', function(keybindings, category_name, module_name) end)

Correct me if I'm wrong, but for something like this we'd need to call this function separate from any module, and define everything there for all modules, similar to what we had before. I really prefer to have each module define its own config and binds.

Have you tested startup speeds and if so how does it perform?

I didn't, but I also didn't notice any slow down anywhere. I will post the startup time later, but bear in mind I'm on a very high end SSD, it's likely that even a slow config would be fast.

How much work will it take to ensure compatibility?

For doom_{modules,userplugins}.lua, it's a simple matter of not wrapping the return in a table with source, instead just returning the the list of modules/plugins. The lang key of modules was moved to the table doom.modules.editor.lsp.servers = { [lang] = { server1, server2, ... } }, and should be removed. doom_config.lua had bigger changes:

At least to me, this seems like stuff we could solve with migration script and a guide for anything not scriptable.


Also, thank you for the support and attention, your work on nest.nvim and reviewing these changes is also awesome.

LuigiPiucco commented 2 years ago

I've just done a (very simple) measure of startup time, I get ~120ms on average (this is the number that's printed to dashboard). For comparison, if I switch to NTBBloodbath/develop I get ~108ms, and that's with the default config, which is pretty clean of modules. The measure of my branch (slightly modified from what's pushed) had all the modules I use, which is most of them. Regardless of numbers, both fire up pretty much instantaneously, or at least faster than I can perceive them.

connorgmeehan commented 2 years ago

Nice it seems pretty good regarding startup times. I wasn't sure if requiring more files because the configs are split into packages, init and binds would add much overhead. Also going over this again, I'm just so keen for the imperative config. Great work!

I couldn't find this example in the required module, check if it hasn't been rebased, or give me a line reference.

Ahh, the HTTP treesitter parser must have been merged upstream. Similar issue though, here's an example of neorg config in the doom-treesitter.lua file.

Unless I'm wrong about packer, nest.nvim is loaded and configured only once. ...

You are right but that would mean we have to load whichkey / nvim-mapper (and nvim-telescope) on start which would slow startup times (telescope is especially bad). Ideally we keep these modules lazy loaded but this doesn't need to be implemented now.

Correct me if I'm wrong, but for something like this we'd need to call this function separate from any module, and define everything there for all modules, similar to what we had before. I really prefer to have each module define its own config and binds.

Maybe this is a better example. With doom-emacs each language or plugin could install and setup it's own mode but the neovim plugin ecosystem just re-uses a couple generic plugins, you just use treesitter + lspconfig for the most part. I think it's fair to extend the API of the module architecture to install/configure treesitter/lsps.


I think addressing these issues can be done once everything else is working. Let me know if you need a hand with anything, I could contribute keybindings if you like? Also let me know if you run into issues with my nest.nvim fork :)

LuigiPiucco commented 2 years ago

I discovered that setting binds in the nest config has issues with modules that only load when you use them, they cause errors on startup. To try and fix that, I'm changing things around a bit, it may also cater to this comment:

Ideally we keep these modules lazy loaded

Instead of having a per module binds file and loading all in nest's config at the end of the load cycle, it will load nest first (after mapper and whichkey), but not set binds yet. Each package config will now be responsible for setting its binds, if it has any. binds.lua is removed (unless you think we should implement something similar to the packer_config table for binds, but it still needs to be loaded by the package's config). This gives finer granularity and solves lazy loading plus load sequencing, at least in theory.

extend the API of the module architecture to install/configure treesitter/lsps.

I see what you meant now. As I had planned it, the user would list which lsps/daps/parsers they wanted in the relevant doom.modules[category][module], and the lsp/dap/required modules would pick up on it and install them. We could add a per lang module, which exports settings to the modules, but I think that would have even more cross module interactions. About neorg, it has its own module now, but it could be directly integrated in this lsp autoconfig I planned, to keep things symmetric, if we go with that. One advantage of per lang modules is easier per lang binds, but it's also more costly to maintain such modules than to do it in lsp. I almost want to go with more modules, but I also don't want to incur extra maintainership cost on the repository. I will wait for you feedback.

Also let me know if you run into issues with my nest.nvim fork

It's been working great, even though I did find a minor regression: original nest.nvim handles something like mode = "nv" by setting inner binds for both modes, yours throws an error if I do that. Other than that, it's pretty good.

LuigiPiucco commented 2 years ago

though I did find a minor regression: original nest.nvim handles something like mode = "nv" by setting inner binds for both modes, yours throws an error if I do that.

Actually, I learned that this only happens if the nvim-mapper integration is enabled, and that it does set the binds, you probably just forgot to implement string iteration in the integration. See here for how it is implemented for the default integration. Since we have 13h difference, I will fork your implementation to continue working, you can later copy and paste from the fork. I also had to incorporate the mode in the uid passed to mapper.

connorgmeehan commented 2 years ago

Oh you're right, thanks for pointing that out. It's still a little buggy but I'll tidy it up properly for release :).

LuigiPiucco commented 2 years ago

So, about the earlier measures on startup time, I discovered the simple measurements I did weren't at all useful. The way it was working is by measuring startup time during dashboard's packer config, but that's a measure of how long it takes for dashboard to load (not even draw), which gives a much smaller number than actual startup time. I also learned that neovim has a CLI flag --startuptime file.log that prints detailed initialization times, and by doing that it turns out my fork actually does run slower (about 2x time-to-display). This is something in dire need of fixing. First, I'll summarize the results, the full logs are here. The order of startups is the same as below.

  1. Startup to dashboard (no file read on startup):

    develop branch: 380.628ms vs. module branch: 318.354ms

    This is somewhat surprising. Without loading the modules (I added BufReadPre as event to packages you won't need until a file is opened), my branch loads faster.

  2. Startup to file with few modules enabled:

    develop branch: 566.671ms vs. module branch: not measured

  3. Startup to file with most modules enabled:

    develop branch: 580.549ms vs. module branch: 1374.962ms

    Here's where the problem is. As soon as the file is opened, all those plugins bound to BufReadPre need to load, and it takes a huge time to do so.

As a solution to this, since I'm already working on autocmds, I think I will start by using conditional autocmds to load packages, possibly also use a delayed vim.defer_fn on BufReadPre to allow a first paint to happen before load for some packages. That being said, I don't know if this will be enough, so I'm asking for suggestions on lazy-loading.

Also, a minor problem I've encountered with keybinds is that they only load once their package loads, I'm trying to think of a way to make the keybinds outside of config, I'd appreciate if someone could look at my branch and help with that.

connorgmeehan commented 2 years ago

I'm happy to have a look into optimising/lazy loading, I've cloned your branch but I'm having some trouble running it. I had to remove module.source from doom_modules.lua to fix utils.is_module_disabled. When re-running :PackerSync it looks like only 11 plugins are being installed, only the ones from core/required/packages.lua.

As for keybinds, I had a look at your source but I'm not sure what the issue is. Can you provide some more information? From my understanding the binds.lua file is read for all enabled doom modules. This is put into doom.binds and bound in required/init.lua on startup.

caligian commented 2 years ago

I think doom-emacs' file layout is too bloated to make sense. It is all over the place and discourages with user making changes to core features. This has led to problems for me before, I don't prefer a framework without a great deal of freedom.

I propose a framework that is functional and self-contained in a way that can be utililized by doom and the user in the same manner to configure or override defaults with ease.

We can have a set of functions that do the following

  1. bootstrap.lua - provides packer.nvim bootstrapper and other package utilities
  2. packages.lua - a set of packer forms hashed using github repo url. Alternatively, it can simply be a list of package declarations via strings.
  3. package_config.lua - hashtable containing a packer form with configuration. The hashtable will be global. So if package_config.lua is loaded, it will overwrite the hashtable and add more lazy loading. This is equivalent to doom's after! macro that lazy-loads package configurations.
  4. lspconfig.lua - Sets up a default lsp configuration with a hashtable for different lsps which can be overridden by users via the global table's modification. Moreover, also add a function that just allows users to add their lsp using the defaults set if they wish to only setup servers without modifying nvim-lsp config. Then declaratively define all lsp configurations (if any) and override nvim-lspconfig from the hashtable from packages.lua. This will lazy load all lsps when nvim-lspconfig is loaded.
  5. settings.lua which contains only vim.opt configs.
  6. init.lua which overrides default configurations if users have made their own changes and additionally merges user package configurations with default package declarations. Only after this, packer.nvim will load all the packages. Users can also disable default packages which will be easy because it will only unset the hooking functions of that package.
  7. utils.lua - Contains important helper functions

The same layout for $DOOMDIR.
Users can either define new functionality or override default functionality and simply add their configs which can be required.

Using a functional interface by ensuring that user and system packages are declarations hashed by the same key in the same global table, makes it easy to reload all configs by a simple function because core/init.lua is just a serially execution of required functions which read tables and do their stuff. So if users use the same ordering of executing, they can override anything or add their own stuff and leave the default system unaffected. This is ultimately much more modular than doom-emacs

Therefore, we can create a very simple file layout.

We can make a doom field in _G (neovim's global table) and add all settings and utilities into it.

caligian commented 2 years ago

Moreover, distinguishing modules according to their functionality such as ui, nav, etc is not all that helpful from a configuration point of view. Usually, you would just get the plugin and declare it and get done with it by a simple descriptive comment above the declaration. But we can do the distinguishment only for the user to observe the differences and then directly modify the plugin according to the plugin's repo instructions.

I am saying this because there is no need to setup a doom variable wrapper for anything because it just increases our work and does nothing for the user who can simply refer to the plugin documentation (which is ultimately what you do when you are serious about a plugin)

Making a nested-tabled table with categories containing their configurations and then iterating through them will only eat up speed when all you are doing is editing the plugin's public api

caligian commented 2 years ago

I will post a full working config in a few days

connorgmeehan commented 2 years ago

I have had some more thoughts on the architecture for this and I'm interested in your thoughts.

Currently we have packages.lua defining plugin dependencies, init.lua defining configuration functions, binds.lua defining keybindings andautocommands.lua defining autocommands. An alternative could be to include all of these fields in init.lua and move the plugin config functions into seperate files. I.e.

local module = {}
module.settings = {
    settings field
}
module.packages = {
  ['pkg-name.nvim'] = { .. packer config .. }
}
module.configs = {
  ['pkg-name.nvim'] = require('... pkg config ...')
}
module.keybinds = { ... nest config ...}

There's a couple things that I think this would improve

But yeah I'd love to see your proposal in action even if it's a minimal example! :)

LuigiPiucco commented 2 years ago

Hello again. I'm sorry to keep silent during these last few comments, I was sound asleep due to my time zone. Anyway, I'll try to answer all of it, so beware the text dragons.

First, about my request for help with lazy loading and @connorgmeehan's troubles running my branch, I think I managed to fix things in a rebase, that's why you can't find it... Sorry for not mentioning it yesterday. I'll detail what I did and the benchmarks separately later. Also, there's more info in my commit message, but doom_modules.lua should just return the list of modules, not a table with a modules field.

Now for @caligian's comments.

I propose a framework that is functional and self-contained in a way that can be utililized by doom and the user in the same manner to configure or override defaults with ease.

I like this idea, in fact it's what I tried to implement, but we can discuss whether I succeeded and how to make it work.

hashed using github repo url

I think just the package name is better here, with the repo url as a subkey. This makes it easier to override with a fork, because you don't have to know which version doom uses, just the package name, which probably won't change across forks.

package_config.lua

This seems similar to the doom.packages key in my implementation, just broken up into a separate file, I assume per module. That can be done very easily. I chose an iteration over packages.lua because it occurred to me first, but this does look more fitting.

We just have to be careful with the configs passed to packer, they should run one after the other, and not override previous functions.

lspconfig.lua

I haven't implemented lsp wrapping yet, so this is the best time to discuss it. I thought something like a module per language, which provides 1. default config for lsp; 2. Extra relevant keybinds; 3. Extra packages, such as lua-dev.nvim, instead of having it in lsp.

settings.lua

Sure, doom.vim is there to do this, but having a separate file that just calls the vim object directly is equivalent. I think in my implementation you can already just call vim variables in doom_config.lua, in hindsight doom.vim is likely superfluous.

init.lua

This is where we would iterate and actually set the configs, correct?

The same layout for $DOOMDIR.

I like this, maybe even provide ways for users to create their own modules using the same structure doom uses internally.

makes it easy to reload all configs by a simple function because core/init.lua is just a serially execution of required functions which read tables and do their stuff.

I'm not positive this would work for reloading, because there may be packages whose setup doesn't work twice, but maybe it always works.

distinguishing modules according to their functionality such as ui, nav, etc is not all that helpful from a configuration point of view

I assume here you mean the category part of doom[setting_type][category][module]? If so, it's easily removed, I just didn't feel like it was worth changing this. We can even do doom[module][setting_type] if agreed upon.

Now back to Connor.

An alternative could be to include all of these fields in init.lua

I initially tried this, but there's an issue with execution order. autocmds and binds depend on doom already being overriden, so they need to be loaded after doom_config.lua.

Instead of manually mapping packages.lua etc to doom.packages['category']['module_name'], iterate over module fields and add every key to doom.

Just as flattening the category out is easy, so is flattening the module out.

doom/core/modules.lua

I assume you mean doom/modules/init.lua? That's where I iterate configs. doom/core/config/init.lua is also relevant, it creates the doom global and runs the overrides. By the way, the user should not need to change doom's code, or at least I tried to make sure they don't.

making pretty much everything configurable from a doom_startup.lua file

Just as a minor correction, doom_startup.lua was the closed PR, now you can just append code to a package's config function, from doom_config.lua.


Over all, thanks to both of you for the contributions.

LuigiPiucco commented 2 years ago

I updated the gist with a new log file for my branch. Startup times are 306.834ms (into dashboard) and 454.028ms (into file). It looks good to me now.

For what was changed, I removed almost all BufReadPres and left just lazy-loaded cmds and modules. Then, I made a change to keybinds, previously they were loaded in the config of the relevant package, now they're loaded by nest's config. However, this is done in a fully lazy way, nest doesn't depend on the packages to set binds. Because I set the appropriate keys in packer's spec, the package are only loaded when used. The exception is lsp, which you want early, but it slows the first draw considerably. For this, I used a ++once autocmd on BufReadPre with a vim.defer_fn that loads the modules after some time. Of course, the change to keybinds means they're always available, not only after their package loads.

LuigiPiucco commented 2 years ago

I did a new force push, it has 2 commits now. For the first one, functionality is kept intact, but I flattened out the layout to remove the category grouping. Current doom global structure is:

doom_modules.lua now just returns a list of modules, no categories.

For the second one, I started a prototype lsp wrapper. I removed lsp and dap install in the first commit, and added it back in the second under a new module, auto_install. Then there's the lua module, which actually adds lua stuff, such as lua-dev.nvim and sumneko_lua config. It also handles auto install, if the module is enabled. The idea is that language modules aren't treated differently. We could add one for each language we would like, and also a default one for simple user wrappers. Further, in later revisions, this makes it easier to allow the user to have their own modules in $DOOMDIR.

See further details in both commit messages.

caligian commented 2 years ago

Started a new discussion with a working demo: https://github.com/NTBBloodbath/doom-nvim/discussions/276

LuigiPiucco commented 2 years ago

I pushed again, with 4 commits now. The first and second just got a few touches, by removing some leftovers and fixing a few things.

The third commit adds hot reloading. It's working pretty well, I made it do a full reload on changes to doom source or $DOOMDIR, it's surprisingly not slow, as I would have expected. There are a few issues with lazy-loading, every once in a while I have to PackerSync after a change, but overall good. I also changed all data directories (plugin/packer_compiled.lua, sessions dir and undo dir) to stdpath("data"), removing the .gitignore entries in the process. Now nothing is stored inside our source code, which hopefully will make updating easier and not capable of deleting anything important. I kept the contribute dir, because I'm not sure what it's for, and I don't know enough docker to try and guess.

The fourth commit enhances autocmd syntax based on pieces of code by @caligian, in their repo, so full credit to them. Now we can pass lua functions as commands, they're added to a hidden global table with an always incrementing id and the autocmd calls this hidden entry.

Next, I'll work on a nvim-search-and-replace module (for #271) and a telescope-project module, then on allowing the user to create custom modules outside doom source code. I'd also like feedback from others about the idea of merging some basic modules, like autopairs, illuminate, gitsigns and indentlines into core, since I expect most users to want these. The idea came from looking at caligian's code, they seem to have done this.

I'll ping people who were interested before, in case they want to provide critique: @connorgmeehan @NTBBloodbath

Also, no rush to answer, I know we're all on holidays. I'm just pinging because my implementation is starting to take shape, and it would be better to know if it needs a different heading sooner than later.

connorgmeehan commented 2 years ago

Hey Luigi and Caligian and happy new year!

Great work so far! I'm thinking it's best to finalize the module architecture and create a PR + explanation of your implementation so we can code review and critique that. Then you can implement the search and replace/telescope project modules in seperate PRs. This would also let me start working on adding my own langs/modules + a documentation generator (READMEs, native vim docs, neorg) for the modules. Also I want to investigate if it's worth putting the reloader into it's own module.

contribute/ contains some tools for contributing, such as a script to update the pinned packer dependencies commit sha to latest (This is something I'll need to update to work with the new architecture as well) + a script to bootstrap a docker environment for development so it's good that you've kept it.

While I do think most people will want these plugins I still think it's worth keeping them separate just for ease of disabling/replacing with own plugin but @NTBBloodbath might have another opinion.


@caligian This setup is very cool and very well written but I think doom-nvim needs to keep modules in seperate files if just for maintainability and scalability. The current ideology behind modules is that they're self contained groups of plugins + configs (+ additional stuff like key-binds and auto-commands) that enable certain functionalities. While your config gives fine grained control of packages and configs, I don't think it follows the philosophy of this project. That being said I really like the simplicity of it and I'd like to channel that into doom-nvim by making it as logical and easy to understand as possible. EDIT: This is just my opinion of course and I'm not ruling out incorporating some of the ideas into doom-nvim but I'll have to take a better look at everything in the new year.

LuigiPiucco commented 2 years ago

Happy new year!

I'll add some documentation and clean up for the PR, then. For now, I think it's best to add it to a new branch, maybe "next" or "v4.0.0", because this will break configs. Later I can write a migration script + guide as well.

I've already done the project module (ended up going with project.nvim), but I'll keep it separate. About search and replace, neither option that I know was to my liking, so I'm trying to roll out my own package (two things were missing in the alternatives, undo support and the doom-emacs-style editable buffer of matches).

About the reloader, it could be added to a module, since it happens on autocmd. It's currently part of core, but we can investigate it after opening the PR.

LuigiPiucco commented 2 years ago

Also, a question for @NTBBloodbath: is it ok to remove the file and doc headers (such as this and that)? I messed up most of them, and I think it's bad practice to have these, it makes the file larger and only holds info that git is better at holding.

NTBBloodbath commented 2 years ago

Hey, happy new year! :)

While I do think most people will want these plugins I still think it's worth keeping them separate just for ease of disabling/replacing with own plugin but @NTBBloodbath might have another opinion.

I don't personally like this, I completely agree that most users would want these plugins (that are enabled by default) but they should have freedom of disabling the plugins and using another ones IMO.

I'll add some documentation and clean up for the PR, then. For now, I think it's best to add it to a new branch, maybe "next" or "v4.0.0", because this will break configs. Later I can write a migration script + guide as well.

Hmm we could use next for the branch name ig, sounds cool lol.

Also thanks for the interest in making a guide to migrate. Let me know if you want me to test something :)

Also, a question for @NTBBloodbath: is it ok to remove the file and doc headers (such as this and that)? I messed up most of them, and I think it's bad practice to have these, it makes the file larger and only holds info that git is better at holding.

Feel free to eliminate the doc headers. They're not very helpful tbh and yeah they're just making files larger.

LuigiPiucco commented 2 years ago

I finished the clean up and pushed. It would seem that a branch must exist on this repo for me to pull request, so could you branch "next" out of "develop"?

NTBBloodbath commented 2 years ago

Great!

so could you branch "next" out of "develop"?

Sure thing, gimme a second :p

Edit: done

LuigiPiucco commented 2 years ago

It's here, folks: #277

connorgmeehan commented 2 years ago

Closed due to v4.0.0 alpha changes in next branch.