Timeout fixes, round 2

[amadeus]

So, despite some changes upstream to ddc, I was still experiencing timeouts, and realized there was a bit of a flaw in my original thinking for how to fix this issue.

The short of it - for every call to ddc#ale#get_completions, we actually need to be sure we fire the notify callback before calling back into Ale for the subsequent request. The reason for this - Ale will simply disregard previous callbacks, and prep with the new ones. While DDC did fix cases where gatherCandidates will never return, it never accounts for the failure of the denops.call failure. Essentially the once callback MUST be executed or else we'll see timeout errors for that registration.

I refactored both the VimL code and TypeScript code to work better inside of this system.

For starters, in the VimL code, I hold onto the callback info locally, and then if I get a new request for completions, I fire the callback immediately with a null result. I then updated the once handler to accept this result and handle it properly, by resolving the promise with an empty array.

As a side note, I realized even though I previously updated the ddc_vim dependencies, there was actually even newer versions for us to use, so did that cleanup as well.

[amadeus]

There's probably another way I can fix this, so if you're not a fan of this solution, I can probably dig more into the denops APIs make better use of async/await stuff and maybe not depend so much on the viml code to handle things.

[Shougo]

To use ddc#refresh_candidates() is better than denops#notify() call.

[amadeus]

To use ddc#refresh_candidates() is better than denops#notify() call.

@Shougo I can certainly look into making this change, can you speak a little bit what the advantages of it are?

At first glance, it feels a lot more complicated, I would argue that the notify version we have here is easier for people to grok when reading the code:

1) gatherCandidates makes a call into vim land to get the results
2) Waits for results to come back
3) When they come back, clean them up and return, or if there's an error or they are empty, return an empty array instead

If we moved to the refresh_candidates version, then gatherCandidates becomes a lot harder to follow, requiring you to jump back and forth between different files to understand what is happening:

1) call gatherCandidates for the first time
2) No previous call or results, so call into the vim code to execute language server fetch
3) Return empty array
4) Sometime later, the LSP responds (in vim land), we set a local vim variable to hold onto the results, and call `refresh_candidates`
5) Gather candidates is called again, due to the local vim variables, we know we have results waiting, so grab them from vim land and return them, instead of an empty array

Said another way: from a developer's perspective, and thinking about future maintainability - the refresh_candidates version, I have to understand the vim code to understand the typescript code. With our version, I don't necessarily NEED to know anything about the vim code - it's abstracted away from me in a function that follows a mostly linear path.

That said, perhaps there's a perspective you have on where you plan to take ddc and perhaps these notify APIs are going away? Or perhaps something else I am missing as well

[Shougo]

If we moved to the refresh_candidates version, then gatherCandidates becomes a lot harder to follow, requiring you to jump back and forth between different files to understand what is happening:

Yes. The callback process is complicated. But it is async.

That said, perhaps there's a perspective you have on where you plan to take ddc and perhaps these notify APIs are going away? Or perhaps something else I am missing as well

The notify API exists, but it is hacky and the spec may be changed. I cannot guarantee it works.

https://github.com/amadeus/ddc-ale/blob/additional-timeout-fix/plugin/ddc/ale.vim

You can remove the file and

https://github.com/amadeus/ddc-ale/tree/additional-timeout-fix/denops/ddc/sources

should move to ddc-sources directory.

[amadeus]

Thanks for the info on the update specs for sources, I moved things around and deleted the unneeded stuff.

@statiolake I think for now, these changes are good to go. I'll explore moving things to the refresh_candidates api in another PR, as at the very least, this PR plus recent ddc changes should negate all timeout issues.

[amadeus]

@statiolake so I did some more experimentation today, and I understand why the refresh_candidates is preferred over this - it's because this current setup actually ends up blocking all other sources from rendering. So if you are mixing say buffer or around source, with this one, nothing can show until this gatherCandidates returns.

I have an iteration of this that copies the implementation from here: https://github.com/shun/ddc-vim-lsp/blob/main/denops/ddc-sources/ddc-vim-lsp.ts#L27-L48 and it really is a much faster experience in general.

I don't want to muddy this PR more than it needs to be, but happy to open up a follow up if it's something you'd be open to accepting

[statiolake]

Now it seems fine. Thanks for the great improvement!

happy to open up a follow up if it's something you'd be open to accepting

I'd be happy to merge such PR :) Now I realized why we should use ddc#refresh_candidates() for good performance.