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 support for inlay hints from TypeScript language server #3455

Merged
merged 1 commit into from
Apr 9, 2022
Merged

Add support for inlay hints from TypeScript language server #3455

merged 1 commit into from
Apr 9, 2022

Conversation

shivjm
Copy link
Contributor

@shivjm shivjm commented Apr 8, 2022

This is an inexpert adaptation (mainly duplication) of the recently-added support for rust-analyzer’s inlay hints to ts-ls. Here’s how it looks in JavaScript and TypeScript, with bright backgrounds for the hints so they stand out in the image:

image

The language server suggests a fair bit of flexibility, almost all of which is reproduced in this PR.1 For example, the return new Entity line that the cursor is on in the top window would normally have hints for the parameters too, but I’ve set lsp-javascript-display-parameter-name-hints-when-argument-matches-name to nil, so, for example, passing a status variable as the status parameter suppresses the hint.

Note that the server’s support is itself marked experimental. In particular, I can’t seem to get any enum member value hints, despite the existence of the option. (Hence the commented-out case in lsp-javascript-format-inlay.)

Footnotes

  1. It didn’t seem useful to have separate parameters for JS and TS, so I clubbed them together. I’m open to the idea that someone else might prefer to distinguish between them.

@github-actions github-actions bot added client One or more of lsp-mode language clients documentation labels Apr 8, 2022
@yyoncho
Copy link
Member

yyoncho commented Apr 9, 2022

Inlay hints was added to the core spec, can you compare both? If it matches the spec then we should have lsp-inlay-hints.el in lsp-mode core

@shivjm
Copy link
Contributor Author

shivjm commented Apr 9, 2022

There’s spec-related discussion in a few different places:

The TypeScript language server itself puts it under a file with ‘proposed’ in the name. Its implementation is slightly different. I don’t see any mention of the spec in these:

Maybe for now this should be ts-ls–specific. We could re-evaluate it as and when the server itself adopts the official version. What do you think?

@yyoncho
Copy link
Member

yyoncho commented Apr 9, 2022

We could re-evaluate it as and when the server itself adopts the official version. What do you think?

Sounds good. It will be great if you can take care of this by monitoring ts-ls development so we can drop the ad-hoc protocol implementation in favor of a real one.

Copy link
Member

@yyoncho yyoncho left a comment

Choose a reason for hiding this comment

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

Looks good to me. I won't be that picky about this implementation since it most likely will be short-lived when the core spec implementation is added. In core implementation I would expect lsp-javascript-inlay-hints-timer to go away in favor of using lsp-on-idle-hook. The benefits are that lsp-on-idle-hook will prevent the refresh of inlay hints when the completion dialog is running (thus making emacs more responsive).

clients/lsp-javascript.el Outdated Show resolved Hide resolved
@shivjm
Copy link
Contributor Author

shivjm commented Apr 9, 2022

It will be great if you can take care of this by monitoring ts-ls development so we can drop the ad-hoc protocol implementation in favor of a real one.

Yup, happy to!

Looks good to me. I won't be that picky about this implementation since it most likely will be short-lived when the core spec implementation is added. In core implementation I would expect lsp-javascript-inlay-hints-timer to go away in favor of using lsp-on-idle-hook. The benefits are that lsp-on-idle-hook will prevent the refresh of inlay hints when the completion dialog is running (thus making emacs more responsive).

That makes a lot of sense. I don’t want to unnecessarily bog the interface down. I’ve updated it to use lsp-on-idle-hook instead.

@shivjm shivjm marked this pull request as ready for review April 9, 2022 14:38
@yyoncho yyoncho merged commit e1e2a49 into emacs-lsp:master Apr 9, 2022
@yyoncho
Copy link
Member

yyoncho commented Apr 9, 2022

Thank you! If you have a Twitter account it will be great if you can tweet about it( I will retweet it).

@shivjm shivjm deleted the add-inlay-hints-from-ts-ls branch April 9, 2022 17:36
@shivjm
Copy link
Contributor Author

shivjm commented Apr 9, 2022

Thank you for the gentle and patient guidance. 😊 I unfortunately don’t have a Twitter account, or I would have been happy to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client One or more of lsp-mode language clients documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants