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

Add TypeScript autoimport support for deoplete #2779

Merged
merged 4 commits into from
Jan 1, 2020

Conversation

jeremija
Copy link
Contributor

This should add support for TypeScript autoimport in deoplete.

This also fixes a bug with the current ALE source for Deoplete since ALE limits the number of completions to g:ale_completion_max_suggestions (default 50). When window. is typed, there will be a lot more results than 50, so most will be filtered out. Because of this, when window.Syn is typed SyntaxError will not be shown. I've tried increasing the limit to a higher number, but then TypeScript completions become really slow (possibly because of completion entry details requests), so I've set the is_volatile = True variable to ALE source.

From deoplete docs:

is_volatile
		(Bool)				(Optional)
		If it is True, the source depends on the user input. It means
		that if this flag is set to False, deoplete will cache
		gather_candidates results and will not call gather_candidates
		on each input change. Only on_post_filter method will be
		called on each input change (if implemented).

		Default: False

It would be great if somebody actively using deoplete (and TypeScript) would be able to test this before merging!

@w0rp
Copy link
Member

w0rp commented Sep 20, 2019

Thanks for this! I'll ask around on IRC to see if anyone wants to try this out.

@cirias
Copy link

cirias commented Sep 20, 2019

You are awesome! Just tested and it worked for me!

@thisjeremiah
Copy link

I tested it and it worked when I removed self.is_volatile = True from the deoplete plugin code. If I kept the line, the autocomplete options wouldn't populate.

These are my deoplete settings:

Plug 'Shougo/deoplete.nvim'
let g:deoplete#enable_at_startup = 1
set completeopt-=preview
let g:deoplete#max_abbr_width = -1
let g:deoplete#max_menu_width = -1
let g:deoplete#ignore_sources = {}
let g:deoplete#ignore_sources._ = [
  \ 'tags', 'buffer', 'around', 'local',
  \ 'file', 'member', 'omni', 'link',
  \ 'LanguageClient'
\]
autocmd FileType * call deoplete#custom#option({
  \ 'auto_complete': v:true,
  \ 'auto_complete_delay': 0,
  \ 'smart_case': v:true,
  \ 'min_pattern_length': 1
\ })
autocmd FileType * call deoplete#custom#option('sources', {
\ '_': ['ale']
\})

Maybe it one of these settings that is causing the different behavior?

@jeremija
Copy link
Contributor Author

Just tested your config and I am unable to reproduce this. Which version of deoplete are you running?

@jeremija
Copy link
Contributor Author

jeremija commented Sep 21, 2019

@thisjeremiah I took another look at this, this time I ran it in a clean neovim environment in Docker:

call plug#begin('~/.config/nvim/plugged')
  Plug 'jeremija/ale', { 'branch': 'feature/autoimport-deoplete' }
  Plug 'Shougo/deoplete.nvim', { 'do': ':UpdateRemotePlugins' }
call plug#end()

augroup FiletypeGroup
    autocmd!
    au BufNewFile,BufRead *.ts set filetype=typescript
augroup END

let g:ale_completion_tsserver_autoimport = 1

let g:deoplete#enable_at_startup = 1
autocmd FileType * call deoplete#custom#option('sources', {
\ '_': ['ale']
\})

I added some echom statements for logging and it looks like deoplete keeps calling ale#completion#GetCompletions in an infinite loop wehn self.is_volatile is set to True (l:source is always set to deoplete). See this video for more information:

deoplete

I've already noticed this before and commented about it here:

On a side note, I noticed that the ale#completion#GetCompletionResult function frequently gets called many, many (too many?) times from ale.py.

@jeremija jeremija force-pushed the feature/autoimport-deoplete branch from 4468b09 to 3dce8bf Compare September 22, 2019 07:12
Shuogo, the author of Deoplete, does not recommend using the `is_async`
option:

> I think is_async is not recommended. It is not so useful and broken.
> You should use callback system instead.

Link: Shougo/deoplete.nvim#1006 (comment)

Incidentally, the same thread mentiones an issue started by w0rp:
Shougo/deoplete.nvim#976

The deoplete docs also say is_async is deprecated:

> is_async        (Bool)
>     If the gather is asynchronous, the source must set
>     it to "True". A typical strategy for an asynchronous
>     gather_candidates method to use this flag is to
>     set is_async flag to True while results are being
>     produced in the background (optionally, returning them
>     as they become ready). Once background processing
>     has completed, is_async flag should be set to False
>     indicating that this is the last portion of the
>     candidates.
>
>     Note: The feature is deprecated and not recommended.
>     You should use callback system by
>     |deoplete#auto_complete()| instead.

Link: https://github.com/Shougo/deoplete.nvim/blob/master/doc/deoplete.txt
@jeremija jeremija force-pushed the feature/autoimport-deoplete branch from 3dce8bf to bdb7c74 Compare September 22, 2019 07:27
@jeremija
Copy link
Contributor Author

@thisjeremiah can you test now?

@w0rp
Copy link
Member

w0rp commented Sep 22, 2019

How recently was the newer callback system added?

@jeremija
Copy link
Contributor Author

jeremija commented Sep 22, 2019

It seems that a pretty recent change introduced problems for some people. See:

The is_async has been marked as deprecated 23 days ago according to git blame.

My latest commit message has some more details.

@jeremija
Copy link
Contributor Author

@w0rp
Copy link
Member

w0rp commented Sep 22, 2019

If it's a very recent change, then we should still support is_async for some time, as many people won't bother to update Deoplete for a while.

Maybe we can detect if the newer feature exists or not, and use one thing or the other.

@jeremija
Copy link
Contributor Author

Yeah, it looks like the latest changes in this PR will work only since a very recent commit in Deoplete:

commit 782fdaf0671d3acc0d70920ed7ca521cfdd5578e
Date:   Thu Sep 19 21:12:30 2019 +0900

@thisjeremiah
Copy link

Yes, I updated deoplete and tested with your updated branch. It works for me now!

@w0rp
Copy link
Member

w0rp commented Sep 30, 2019

Okay, I think some detection of the Deoplete version is needed, and maybe we can only enable support for autoimports in Deoplete if the Deoplete version is new enough.

@jeremija
Copy link
Contributor Author

I agree, I'm just not sure what would be the best way to check for this, since the old autocomplete functionality is buggy in the current version of deoplete (is_async callbacks sometimes work, and sometimes do not because it calls gather_candidates in an endless loop of is_async and is_refresh events). It does not seem to matter if autoimport is enabled or not, but it has something to do with the is_volatile flag.

AFAIK, there is no way to retrieve Deoplete version from vim/python - the only place I found it was in doc/deoplete.txt.

If anybody has an idea on how to reliably check for the new feature (or the bug), I'd love to hear it!

@w0rp
Copy link
Member

w0rp commented Oct 7, 2019

I think we can check exists('deoplete#auto_complete') in Vim, via the same mechanism for calling a command.

@jeremija
Copy link
Contributor Author

jeremija commented Oct 7, 2019

I think we can check exists('deoplete#auto_complete') in Vim, via the same mechanism for calling a command.

Unfortunately, this function was in deoplete before Shougo/deoplete.nvim@782fdaf, which is the most recent commit that my fixes work with :/

@w0rp
Copy link
Member

w0rp commented Oct 9, 2019

Maybe there's something else we can use to detect if Deoplete is new enough.

@laino
Copy link
Contributor

laino commented Nov 22, 2019

Unfortunately, this function was in deoplete before Shougo/deoplete.nvim@782fdaf, which is the most recent commit that my fixes work with :/

Using deoplete at that version together with your branch gives me working auto-import functionality. It's really cool.

@w0rp
Copy link
Member

w0rp commented Jan 1, 2020

At this point, I'll merge this, and if anyone has problems they can update Deoplete. The version which works with this is old enough now.

@w0rp w0rp merged commit 0cb432c into dense-analysis:master Jan 1, 2020
@w0rp
Copy link
Member

w0rp commented Jan 1, 2020

Cheers! 🍻

anupdhml pushed a commit to anupdhml/ale that referenced this pull request Feb 4, 2020
* Add autoimport support for deoplete

* Fix test_deoplete_source.py

* Use callback instead of is_async for deoplete

Shuogo, the author of Deoplete, does not recommend using the `is_async`
option:

> I think is_async is not recommended. It is not so useful and broken.
> You should use callback system instead.

Link: Shougo/deoplete.nvim#1006 (comment)

Incidentally, the same thread mentiones an issue started by w0rp:
Shougo/deoplete.nvim#976

The deoplete docs also say is_async is deprecated:

> is_async        (Bool)
>     If the gather is asynchronous, the source must set
>     it to "True". A typical strategy for an asynchronous
>     gather_candidates method to use this flag is to
>     set is_async flag to True while results are being
>     produced in the background (optionally, returning them
>     as they become ready). Once background processing
>     has completed, is_async flag should be set to False
>     indicating that this is the last portion of the
>     candidates.
>
>     Note: The feature is deprecated and not recommended.
>     You should use callback system by
>     |deoplete#auto_complete()| instead.

Link: https://github.com/Shougo/deoplete.nvim/blob/master/doc/deoplete.txt

Co-authored-by: w0rp <w0rp@users.noreply.github.com>
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.

5 participants