Track loading states of providers #9914
Conversation
felixfbecker
added
team/web
extensions
felixfbecker
force-pushed
the
provider-loading-states
branch
from
80683b2
to
425a7a9
Compare
| @@ -356,8 +357,6 @@ function initCodeIntelligence({ | |||
| } { | |||
| const subscription = new Subscription() | |||
|
|
|||
| const { getHover } = createLSPFromExtensions(extensionsController) | |||
There was a problem hiding this comment.
I love that we're deleting this 😄
| // Merge partial updates | ||
| scan( | ||
| (previous, current) => ({ ...previous, ...current }), | ||
| ((): MaybeLoadingResult<UIDefinitionURL | null> => ({ isLoading: true, result: null }))() |
There was a problem hiding this comment.
Why is the IIFE necessary here?
There was a problem hiding this comment.
Without it, the scan()'s output type parameter R would be inferred to { isLoading: boolean, result: null }, because it is inferred from the seed parameter (if provided). This is just a type hint for the actual type. Alternatives would be to specify the generic type parameters <T, R> but that would require repeating the type twice (because T = R), or using a cast, but that is unsafe. I know IIFE can look overly complex at first but it's actually the simplest & safest solution 😄
There was a problem hiding this comment.
otherwise, a emptyLoadingResult variable should also work, this is indeed the simplest inline solution, but it's rather hard to understand on first sight, no? 😁
There was a problem hiding this comment.
Yeah, that would work, I just wanted to avoid the definition being far away from the usage (it would have to be at the top of the file). My hope is that by using this pattern more often in the codebase it'll feel less alien and it's just one of those things that look odd at first but you get used to it quickly. But if you both feel strongly this impedes readability let me know and I'm happy to change it to the <T, R> generics solution (which is possible here, but not everywhere)
| * | ||
| * @param constant The value to compare to. | ||
| */ | ||
| export const isExactly = <T, C extends T>(constant: C) => (value: T): value is C => value === constant |
| // Merge partial updates | ||
| scan( | ||
| (previous, current) => ({ ...previous, ...current }), | ||
| ((): MaybeLoadingResult<UIDefinitionURL | null> => ({ isLoading: true, result: null }))() |
There was a problem hiding this comment.
otherwise, a emptyLoadingResult variable should also work, this is indeed the simplest inline solution, but it's rather hard to understand on first sight, no? 😁
Co-Authored-By: Loïc Guychard <loic@sourcegraph.com>
Codecov Report
@@ Coverage Diff @@
## master #9914 +/- ##
=======================================
Coverage 42.76% 42.76%
=======================================
Files 1347 1347
Lines 73949 73929 -20
Branches 6641 6625 -16
=======================================
- Hits 31625 31617 -8
+ Misses 39464 39452 -12
Partials 2860 2860
|
Fixes #9349, fixes #9350. Depends on sourcegraph/codeintellify#251.
We considered the providers loading in the time between the invocation and the first emission. This is a problem, because if no providers were registered yet at invocation time, an empty result will be emitted. When providers are then registered afterwards and start loading, we don't show a loading indicator, because we had no way of knowing they were loading. It was supposed to be solved by using higher-order Observables (
Observable<Observable<T>>), but they weren't piped through the whole stack, but flattened relatively quickly. I decided on the explicit object wrapper type instead.This uses the
MaybeLoadingResult<T>type added in sourcegraph/codeintellify#251 and updates the loading logic to use the overhauled, abstractedemitLoading()operator from that PR. This allows us to track throughout our stack when a result stream starts loading again. This is then used in the definition logic, passed to codeintellify for the hover logic, and also for locations in the reference panel (so unregistering/reregistering a provider should now show a loader again @sourcegraph/code-intel). The "loading again" indication is not exposed through the extension API, but that could now easily be added if we have the use case (@sourcegraph/code-intel ?).I updated all the Rx marble tests to conform to the new emission type, and added new tests that specifically test the previously missing cases of registering a provider after an empty result was already emitted. I did my best to add comments everywhere to explain them better. I also tested manually and ran the hover e2e tests.
A side note: Updating all these tests was a lot of effort (I spent Monday-Thursday on this PR, with tests taking up more than 1 full day). There are a lot of tests here that test redundant things because implementations are not abstracted well (e.g.
getHover()andgetLocations()do almost the same thing). Some tests also cover a lot of cases of handlingnull, even though there is no functional difference between anullresult and an empty array result. I partly removed thenulltype in favor of empty arrays, but the providers from the extension API still are allowed to returnnull. Because the implementation is more akin to a nested call graph as opposed to individual functions that are composed at the top level, the tests for the top-level functions still have to test this low-level behaviour. Lots of obvious potential for cleanup. As we invest more into our extension API, I foresee these code smells to slow us down the same way.cc @sourcegraph/code-intel @beyang