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

Fall back to basic-code-intel after 500ms #140

Closed
wants to merge 1 commit into from
Closed

Fall back to basic-code-intel after 500ms #140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 87 additions & 33 deletions extension/src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { URL as _URL, URLSearchParams as _URLSearchParams } from 'whatwg-url'
Object.assign(_URL, self.URL)
Object.assign(self, { URL: _URL, URLSearchParams: _URLSearchParams })

import { activateBasicCodeIntel } from '@sourcegraph/basic-code-intel'
import * as basicCodeIntel from '@sourcegraph/basic-code-intel'
import { Tracer as LightstepTracer } from '@sourcegraph/lightstep-tracer-webworker'
import {
createMessageConnection,
Expand All @@ -20,7 +20,7 @@ import { merge } from 'ix/asynciterable/index'
import { filter, map, scan, tap } from 'ix/asynciterable/pipe/index'
import { fromPairs } from 'lodash'
import { Span, Tracer } from 'opentracing'
import { BehaviorSubject, from, fromEventPattern, Subscription } from 'rxjs'
import { BehaviorSubject, from, fromEventPattern, Observable, ObservableInput, of, race, Subscription } from 'rxjs'
import * as rxop from 'rxjs/operators'
import * as sourcegraph from 'sourcegraph'
import { Omit } from 'type-zoo'
Expand Down Expand Up @@ -95,6 +95,58 @@ const documentSelector: sourcegraph.DocumentSelector = [{ language: 'typescript'

const logger: Logger = new RedactingLogger(console)

/**
* Emits from `fallback` after `delayMilliseconds`. Useful for falling back to
* basic-code-intel while the language server is running.
*/
function withFallback<T>({
main,
fallback,
delayMilliseconds,
}: {
main: ObservableInput<T>
fallback: ObservableInput<T>
delayMilliseconds: number
}): Observable<T> {
return race(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think race() gives the wrong behaviour here. As described in #107, I think the behaviour should be to show the basic code intel result while the language server hasn't returned yet, but (for hovers/references) replace it as soon as the language server has a result. The delay would just be there to avoid a flickering UI (rapidly changing from basic code intel to language server) and for definition, where we can't replace the results afterwards because we need to jump to the location.

Without this, it's too easy for a hover or references call to take slightly longer than 500ms or 2s respectively and the user getting imprecise results (or maybe even no result at all).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, continuing conversation in #107

of(null).pipe(rxop.switchMap(() => from(main))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this the same?

Suggested change
of(null).pipe(rxop.switchMap(() => from(main))),
main,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's the same as main. The way I wrote it, the difference between this and the fallback is more obvious, namely that the fallback is delayed.

If it gave you pause, I'll switch to main.

of(null).pipe(
rxop.delay(delayMilliseconds),
rxop.switchMap(() => from(fallback))
)
)
}

const basicCodeIntelHandlerArgs: basicCodeIntel.HandlerArgs = {
sourcegraph,
languageID: 'typescript',
fileExts: ['ts', 'tsx', 'js', 'jsx'],
commentStyle: {
lineRegex: /\/\/\s?/,
block: {
startRegex: /\/\*\*?/,
lineNoiseRegex: /(^\s*\*\s?)?/,
endRegex: /\*\//,
},
},
filterDefinitions: ({ filePath, fileContent, results }) => {
const imports = fileContent
.split('\n')
.map(line => {
// Matches the import at index 1
const match = /\bfrom ['"](.*)['"];?$/.exec(line) || /\brequire\(['"](.*)['"]\)/.exec(line)
return match ? match[1] : undefined
})
.filter((x): x is string => Boolean(x))

const filteredResults = results.filter(result =>
imports.some(i => path.join(path.dirname(filePath), i) === result.file.replace(/\.[^/.]+$/, ''))
)

return filteredResults.length === 0 ? results : filteredResults
},
}

export async function activate(ctx: sourcegraph.ExtensionContext): Promise<void> {
// Cancel everything whene extension is deactivated
const cancellationTokenSource = new CancellationTokenSource()
Expand All @@ -104,37 +156,26 @@ export async function activate(ctx: sourcegraph.ExtensionContext): Promise<void>
const config = new BehaviorSubject(getConfig())
ctx.subscriptions.add(sourcegraph.configuration.subscribe(() => config.next(getConfig())))

const basicCodeIntelHandler = new basicCodeIntel.Handler(basicCodeIntelHandlerArgs)

if (!config.value['typescript.serverUrl']) {
logger.warn('No typescript.serverUrl configured, falling back to basic code intelligence')
// Fall back to basic-code-intel behavior
return activateBasicCodeIntel({
languageID: 'typescript',
fileExts: ['ts', 'tsx', 'js', 'jsx'],
commentStyle: {
lineRegex: /\/\/\s?/,
block: {
startRegex: /\/\*\*?/,
lineNoiseRegex: /(^\s*\*\s?)?/,
endRegex: /\*\//,
},
},
filterDefinitions: ({ filePath, fileContent, results }) => {
const imports = fileContent
.split('\n')
.map(line => {
// Matches the import at index 1
const match = /\bfrom ['"](.*)['"];?$/.exec(line) || /\brequire\(['"](.*)['"]\)/.exec(line)
return match ? match[1] : undefined
})
.filter((x): x is string => Boolean(x))

const filteredResults = results.filter(result =>
imports.some(i => path.join(path.dirname(filePath), i) === result.file.replace(/\.[^/.]+$/, ''))
)

return filteredResults.length === 0 ? results : filteredResults
},
})(ctx)
ctx.subscriptions.add(
sourcegraph.languages.registerHoverProvider(documentSelector, {
provideHover: (doc, pos) => basicCodeIntelHandler.hover(doc, pos),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if basicCodeIntelHandler implemented HoverProvider, or had a hoverProvider property :)

})
)
ctx.subscriptions.add(
sourcegraph.languages.registerDefinitionProvider(documentSelector, {
provideDefinition: (doc, pos) => basicCodeIntelHandler.definition(doc, pos),
})
)
ctx.subscriptions.add(
sourcegraph.languages.registerReferenceProvider(documentSelector, {
provideReferences: (doc, pos) => basicCodeIntelHandler.references(doc, pos),
})
)
}

const tracer: Tracer = config.value['lightstep.token']
Expand Down Expand Up @@ -508,7 +549,11 @@ export async function activate(ctx: sourcegraph.ExtensionContext): Promise<void>
providers.add(
sourcegraph.languages.registerHoverProvider(documentSelector, {
provideHover: distinctUntilChanged(areProviderParamsEqual, (textDocument, position) =>
observableFromAsyncIterable(provideHover(textDocument, position))
withFallback({
main: observableFromAsyncIterable(provideHover(textDocument, position)),
fallback: basicCodeIntelHandler.hover(textDocument, position),
delayMilliseconds: 500,
})
),
})
)
Expand Down Expand Up @@ -544,7 +589,11 @@ export async function activate(ctx: sourcegraph.ExtensionContext): Promise<void>
providers.add(
sourcegraph.languages.registerDefinitionProvider(documentSelector, {
provideDefinition: distinctUntilChanged(areProviderParamsEqual, (textDocument, position) =>
observableFromAsyncIterable(provideDefinition(textDocument, position))
withFallback({
main: observableFromAsyncIterable(provideDefinition(textDocument, position)),
fallback: basicCodeIntelHandler.definition(textDocument, position),
delayMilliseconds: 500,
})
),
})
)
Expand Down Expand Up @@ -736,7 +785,12 @@ export async function activate(ctx: sourcegraph.ExtensionContext): Promise<void>
})
providers.add(
sourcegraph.languages.registerReferenceProvider(documentSelector, {
provideReferences: (doc, pos, ctx) => observableFromAsyncIterable(provideReferences(doc, pos, ctx)),
provideReferences: (doc, pos, ctx) =>
withFallback({
main: observableFromAsyncIterable(provideReferences(doc, pos, ctx)),
fallback: basicCodeIntelHandler.references(doc, pos),
delayMilliseconds: 2000,
}),
})
)

Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@
"yarn-deduplicate": "^1.1.1"
},
"dependencies": {
"@sourcegraph/basic-code-intel": "^6.0.13",
"@sourcegraph/basic-code-intel": "^6.0.14",
"@sourcegraph/lightstep-tracer-webworker": "^0.20.14-fork.3",
"@sourcegraph/typescript-language-server": "^0.3.7-fork",
"@sourcegraph/vscode-ws-jsonrpc": "0.0.3-fork",
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,10 +714,10 @@
resolved "https://registry.npmjs.org/@sindresorhus/is/-/is-0.7.0.tgz#9a06f4f137ee84d7df0460c1fdb1135ffa6c50fd"
integrity sha512-ONhaKPIufzzrlNbqtWFFd+jlnemX6lJAgq9ZeiZtS7I1PIf/la7CW4m83rTXRnVnsMbW2k56pGYu7AUFJD9Pow==

"@sourcegraph/basic-code-intel@^6.0.13":
version "6.0.13"
resolved "https://registry.npmjs.org/@sourcegraph/basic-code-intel/-/basic-code-intel-6.0.13.tgz#1fa7030d4b5813c8ede3a51dacd81647dd33edee"
integrity sha512-psxFy4lj8A8kRH2WMGFam0y1+VrBQqIgrOrTyH6At27Qpxwuz6OlHkKzTBUnc9HqNDkxGAEIuUSx5kMuLYoK4g==
"@sourcegraph/basic-code-intel@^6.0.14":
version "6.0.14"
resolved "https://registry.npmjs.org/@sourcegraph/basic-code-intel/-/basic-code-intel-6.0.14.tgz#898216e8966d1b902c5e49199a2a33d255ed9de4"
integrity sha512-SekYreQAgpmeE4oNWh+Dke6ChXwudLyr+5qU79DX6lSQY1UQLg55T5cmwSbcIBLKwQzvyyUJ7YLvtQhXOn7QwA==
dependencies:
lodash "^4.17.11"
rxjs "^6.3.3"
Expand Down