Skip to content
This repository was archived by the owner on Jun 7, 2022. It is now read-only.

Conversation

@efritz
Copy link
Contributor

@efritz efritz commented Jan 24, 2020

This PR cleans up the init code after sourcegraph/code-intel-extensions#198.

This removes all LSIF and search-based logic and instead delegates to the activateCodeIntel function, passing it only the language-server specific logic.

This PR will be followed by a major cleanup.

@efritz efritz force-pushed the activation-refactor branch from b1a1b23 to c5c166c Compare January 27, 2020 15:08
@efritz efritz force-pushed the activation-refactor branch from c5c166c to a6c21be Compare January 27, 2020 15:14
@efritz efritz requested a review from felixfbecker January 27, 2020 21:52
@chrismwendt
Copy link
Contributor

I got an error with "typescript.serverUrl": ""

image

Passing undefined to the lspProviders argument when that's unset might do the trick.

@efritz
Copy link
Contributor Author

efritz commented Jan 28, 2020

Passing undefined to the lspProviders argument when that's unset might do the trick.

Updated.

Copy link
Contributor

@chrismwendt chrismwendt left a comment

Choose a reason for hiding this comment

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

I verified the hover/def/ref behavior with/without LSIF/LSP remains unchanged (same as sourcegraph/sourcegraph-go#117 (review))

ctx,
documentSelector,
handlerArgs,
config.value['typescript.serverUrl']
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that before, you could change the config during a session and it would be considered. Now it seems the config is read only once during activation. I think this could cause problems in particular in the browser extension, where extensions are run in the background page and are not necessarily re-activated just because the page got reloaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah you're right - I missed that. We used to check on each function invocation. I'll update the new API to support doing this dynamically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants