TypeScript toggle doesn't work if basic code intel fallback is used

[felixfbecker]

This is because the activate function does an early return before the toggle command is registered, and there is no logic to handle the toggle for basic code intel.

The toggle should still work, and ideally communicate in the tooltip or otherwise that basic code intel is used.

cc @chrismwendt

[chrismwendt]

Perhaps we could hide the toggle when basic code intel is used?

[felixfbecker]

The toggle is there to allow disabling hovers mainly on PRs when code intel is unwanted, so it should work when basic code intel is used too

[chrismwendt]

Gotcha, I wonder if this applies to all the other languages, too.

[felixfbecker]

Definitely

[chrismwendt]

Have we gotten feedback about this for other languages? I don't recall off the top of my head.

[felixfbecker]

Not sure, I don't remember the feedback as langauge-specific. We heard the same feedback a lot of times (not usually mentioning any particular language)

[chrismwendt]

Oh, yup https://sourcegraph.slack.com/archives/C0W2E592M/p1550877960027100

Too many popups, too much uploading of my content, and generally too complicated and noisy. I just want see green/red gutters for codecov coverage on GitHub, I don't want to reinvent GitHub.

This makes me wonder if it would be worth adding a global code intel enable/disable toggle (so we don't have to implement it multiple times).

[sqs]

@chrismwendt The toggle should just be implemented in https://sourcegraph.com/github.com/sourcegraph/lsp-client (and that should be used by all language extensions), and then it would not need to be implemented multiple times.

I don't think the global code intel enable/disable toggle is a good idea based on how I am imagining it. And it is not necessary given the above implementation (in lsp-client). If you still think it would be a good idea, can you explain how it would know if a given hover/reference/definition provider or progress notification is "related to code intel"?

[felixfbecker]

Agree. The toggle (and setting) always needs to be defined in package.json for each extension, so both lsp-client and basic-code-intel npm packages should support an option on what setting to listen to (could just use ${language}.enable to make it consistent). They could both register the command implementation (since it's a oneliner) or that could be its own package.

[chrismwendt]

Sounds good, I was thinking of a toggle to disable all hover/def/ref providers, but there might be non-code-intel extensions that use those. There's no way that I can think of to tell if an arbitrary provider is "related to code intel".

The only blocker is that the Go extension will need sourcegraph/lsp-client#11 (which in turn needs refactoring).

[ryan-blunden]

@felixfbecker I'm seeing this when clicking on the [TS] icon:

Is that relevant for this issue or something else?

[felixfbecker]

Yes, that is exactly this issue.

[felixfbecker]

The toggle was removed.