This repository has been archived by the owner on Nov 25, 2021. It is now read-only.
feat: allow hover providers to communicate loading state #251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BREAKING CHANGE: Hover providers that return Subscribables MUST communicate loading state. If the hover provider only emits a single result, it can return a Promise instead.
Codecov Report
@@ Coverage Diff @@
## master #251 +/- ##
==========================================
+ Coverage 84.95% 85.29% +0.33%
==========================================
Files 13 14 +1
Lines 565 578 +13
Branches 141 145 +4
==========================================
+ Hits 480 493 +13
Misses 85 85
Continue to review full report at Codecov.
|
felixfbecker
added this to Needs review
in Web Team :: Current iteration
via automation
Web Team :: Current iteration
automation
moved this from Needs review
to Reviewer approved
Apr 16, 2020
lguychard
approved these changes
Apr 16, 2020
src/loading.test.ts
Outdated
| import { TestScheduler } from 'rxjs/testing' | ||
| import { emitLoading, LOADING, MaybeLoadingResult } from './loading' | ||
|
|
||
| const inputAlphabet: Record<'l' | 'e' | 'r', MaybeLoadingResult<number | null>> = { |
There was a problem hiding this comment.
I love the "shared alphabets" for tests 🙂
Co-Authored-By: Loïc Guychard <loic@sourcegraph.com>
|
Thanks for the thorough review @lguychard! I'll wait with merging this until the main repo PR is approved too so this doesn't turn out to need more adjustments after merging |
eseliger
approved these changes
Apr 16, 2020
|
🎉 This PR is included in version 7.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This was referenced Apr 20, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses sourcegraph/sourcegraph#9349, sourcegraph/sourcegraph#9350 and sourcegraph/sourcegraph#9346.
Codeintellify was originally not written to support providers updating the hover content later. It accepted Observables for cancellation, but they were intended to only emit once.
With extensions, Observables started to emit multiple times to update the content. The problem is that the period between invocation and the first result is used to display a loading indicator. But with multiple emissions, the situation can arise where first an empty result is emitted (e.g. because no providers were registered yet), then providers get registered and start loading, then emit actual non-empty result. Codeintellify had no way to know that providers started loading again.
This changes the types so that when an Observable is returned, it has to contain information about when the provider is loading or not loading in addition to the result to show (through
MaybeLoadingResult<T>). It's still possible to only emit a single result by returning a Promise, in which case this information does not need to be provided because it is known (time between invocation and resolution).This information is then used to make the loading indicator logic smarter - it now shows a loader even if the provider had emitted an empty result before, given that the LOADER_DELAY has elapsed. This logic was also duplicated with only slight modifications for definitions (in the main repo), so it is now abstracted in its own reusable Rx operator, which has Rx marble unit tests.
PR for the main repo: sourcegraph/sourcegraph#9914