Skip to content

Track loading states of providers

Administrator requested to merge provider-loading-states into master

Created by: felixfbecker

Fixes #9349, fixes #9350. Depends on https://github.com/sourcegraph/codeintellify/pull/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 https://github.com/sourcegraph/codeintellify/pull/251 and updates the loading logic to use the overhauled, abstracted emitLoading() 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() and getLocations() do almost the same thing). Some tests also cover a lot of cases of handling null, even though there is no functional difference between a null result and an empty array result. I partly removed the null type in favor of empty arrays, but the providers from the extension API still are allowed to return null. 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

Merge request reports

Loading