Fix withFallback to emit from main even if fallback has emitted #57
Conversation
Fixes #3039 `withFallback()` used `rxjs.race()` to fallback to basic-code-intel if the language server had not emitted after 500ms. The problem is that `race()` works as follows: > The observable to emit first is used. This is an issue when pinning the hover tooltip, because if the basic-code-intel result is used first, the language server result will *never be used*.
b846cbe to
040ccfe
Compare
| @@ -557,12 +558,14 @@ function withFallback<T>({ | |||
| fallback: ObservableInput<T> | |||
| delayMilliseconds: number | |||
| }): Observable<T> { | |||
| return race( | |||
| of(null).pipe(switchMap(() => from(main))), | |||
| const mainObservable = from(main).pipe(share()) | |||
There was a problem hiding this comment.
@chrismwendt this is the best I could come up with to express "emit from fallback after delay, unless main has emitted, and emit everything from main". Happy to take suggestions on this 😄
There was a problem hiding this comment.
I implemented this independently and came up with exactly the same solution 😺
|
Food for thought: could we find a way to test features like falling back to basic-code-intel with unit tests / e2e tests at the extension level, so that we can catch things like this without them causing CI failures on sourcegraph/sourcegraph due to out-of-band extension updates? |
|
Thanks, @lguychard! I plan to commandeer this PR and put the behavior behind a feature flag with 2 behaviors:
I'll gather some feedback before making either of these the default (or not change it at all if I get negative feedback). |
|
Closing because falling back to basic-code-intel mostly reduces confidence and creates confusion, see sourcegraph/sourcegraph-typescript#107 (comment) Maybe a button like "Show 8 fuzzy results" could be added to the references panel when loading takes more than some threshold amount of time. That way it's user-initiated, less implicit, and less confusing. It might get tedious to keep clicking the button, which would require yet another solution. |
Fixes sourcegraph/sourcegraph#3039
withFallback()usedrxjs.race()to fallback to basic-code-intel if the language server had not emitted after 500ms. The problem is thatrace()works as follows:This is an issue when pinning the hover tooltip, because if the basic-code-intel result is used first, the language server result will never be used.