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

fix(compile) schedule loading to after event #636

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AckslD
Copy link
Contributor

@AckslD AckslD commented Oct 6, 2021

As reported in AckslD/nvim-neoclip.lua#26 it seems that autocommands defined in config are called twice if plugin is loaded by an event. To reproduce one can try the following small example:

    use {
        '~/some_path',
        event = 'TextYankPost',
        config = function()
            local count = 0
            _G.test_handler = function()
                count = count + 1
                print('called', count, 'times')
            end
            vim.cmd([[autocmd TextYankPost * lua _G.test_handler()]])
        end,
    }

(where ~/some_path is some existing path)

If you compile this and yank something you will see called 2 times. Unless I'm mistaken this happens since the package (and config) is loaded while handling the event so the autocommand will be added and still executed. But also doautocmd is called which triggers the handler again.

This PR puts the loading in vim.schedule and will thus be handled after the event is "finished".

Copy link
Owner

@wbthomason wbthomason left a comment

Choose a reason for hiding this comment

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

Thanks, and nice job tracking down this issue! I wonder if it might be better to instead use schedule_wrap on the definition of packer.load - if doing so has the same effect (which I expect it should) as this change, I prefer that approach as it's a bit cleaner. What do you think, @AckslD?

@AckslD
Copy link
Contributor Author

AckslD commented Oct 7, 2021

@wbthomason Yeah that's cleaner indeed, however it does not seem to work at least in my setup and breaks other things. I guess because for other loading triggers (ft, keys etc) there is no corresponding doautocmd?

We could add another function somewhere (not sure where exactly) which is

schedule_load = vim.schedule_wrap(require("packer.load"))

which is only used when event is used?

Btw I have to give credit to @tjdevries and his video without which it would have taken me much longer to understand what was going on here :)

@wbthomason
Copy link
Owner

Hmm. schedule_wrap-ing packer.load does work in my testing, and doesn't appear to break things. To make sure we're on the same page, I'm modifying only this line:

return load_wrapper

to read

return vim.schedule_wrap(load_wrapper)

which, with your example spec, prints "called 1 times" as expected (I can reproduce the issue if I don't use schedule_wrap).

Do you have an example of what this breaks for you? I didn't notice any breakage, but it's possible I just didn't test sufficiently extensively.

@AckslD
Copy link
Contributor Author

AckslD commented Oct 8, 2021

@wbthomason Doing the change you mention above seems to break for me to lazy load by module. Does that work for you?

@wbthomason
Copy link
Owner

Hmm, that works for me. For example, my telescope config uses module loading:

    {
      'nvim-telescope/telescope.nvim',
      requires = {
        'nvim-lua/popup.nvim',
        'nvim-lua/plenary.nvim',
        'telescope-frecency.nvim',
        'telescope-fzf-native.nvim',
      },
      wants = {
        'popup.nvim',
        'plenary.nvim',
        'telescope-frecency.nvim',
        'telescope-fzf-native.nvim',
      },
      setup = [[require('config.telescope_setup')]],
      config = [[require('config.telescope')]],
      cmd = 'Telescope',
      module = 'telescope',
    },

and works as expected with this change. Do you have an example of what breaks for you?

@AckslD
Copy link
Contributor Author

AckslD commented Oct 9, 2021

@wbthomason I think what breaks for me is that I have some plugins which gets loaded by module which are in turn called in the config of another plugin.

For example this is my lsp setup:

    use { -- lspconfig {{{
        'neovim/nvim-lspconfig',
        requires = {
            {'ray-x/lsp_signature.nvim', module = 'lsp_signature'},
            { -- {{{ null-ls
                'jose-elias-alvarez/null-ls.nvim',
                module = 'null-ls',
                config = 'require("plugin_settings.null_ls").config()',
            }, -- }}}
        },
        config = 'require("plugin_settings.lspconfig").config()',
    } -- }}}

which inside pluging_settings.lspconfig I call require('null-ls') which triggers the loading. Before and also with the submitted fix this works fine. However adding schedule_wrap(load_wrapper) gives the error:

Error in packer_compiled: /home/axel/.config/nvim/lua/plugin_settings/lspconfig.lua:143: module 'null-ls' not found:

Edit: Maybe it's cause I'm doing something wrong here which is not meant to be supported? I tried to also add wants but got the same error.

@AckslD
Copy link
Contributor Author

AckslD commented Nov 6, 2021

@wbthomason any more thoughts on this? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants