Skip to content
This repository was archived by the owner on Nov 25, 2021. It is now read-only.

Commit 47cb2c5

Browse files
authored
feat: allow hover providers to communicate loading state (#251)
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. 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.
1 parent 2becf28 commit 47cb2c5

File tree

11 files changed

+271
-131
lines changed

11 files changed

+271
-131
lines changed

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
"@sourcegraph/event-positions": "^1.0.4",
3434
"@sourcegraph/extension-api-types": "^2.0.0",
3535
"lodash": "^4.17.10",
36-
"rxjs": "^6.3.3",
36+
"rxjs": "^6.5.5",
3737
"ts-key-enum": "^2.0.0"
3838
},
3939
"devDependencies": {

src/helpers.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,24 @@
1+
import { MaybeLoadingResult } from './loading'
2+
import { Subscribable } from 'sourcegraph'
3+
import { Observable, from } from 'rxjs'
4+
import { isObject } from 'lodash'
5+
import { map } from 'rxjs/operators'
6+
7+
/**
8+
* Checks if the given value is thenable.
9+
*/
10+
const isPromiseLike = (value: unknown): value is PromiseLike<unknown> =>
11+
isObject(value) && typeof (value as PromiseLike<unknown>).then === 'function'
12+
13+
/**
14+
* Converts a provider result, which can be a continuously-updating, maybe-loading Subscribable, or a Promise for a
15+
* single result, to the same type.
16+
*/
17+
export const toMaybeLoadingProviderResult = <T>(
18+
value: Subscribable<MaybeLoadingResult<T>> | PromiseLike<T>
19+
): Observable<MaybeLoadingResult<T>> =>
20+
isPromiseLike(value) ? from(value).pipe(map(result => ({ isLoading: false, result }))) : from(value)
21+
122
/**
223
* Returns true if `val` is not `null` or `undefined`
324
*/

src/hoverifier.test.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@ import { findPositionsFromEvents, SupportedMouseEvent } from './positions'
1818
import { CodeViewProps, DOM } from './testutils/dom'
1919
import { createHoverAttachment, createStubActionsProvider, createStubHoverProvider } from './testutils/fixtures'
2020
import { dispatchMouseEventAtPositionImpure } from './testutils/mouse'
21-
import { HoverAttachment, LOADING } from './types'
21+
import { HoverAttachment } from './types'
22+
import { LOADING } from './loading'
23+
2224
const { assert } = chai
2325

2426
describe('Hoverifier', () => {
@@ -391,7 +393,9 @@ describe('Hoverifier', () => {
391393
hoverOverlayRerenders: EMPTY,
392394
// Only show on line 24, not line 25 (which is the 2nd click event below).
393395
getHover: position =>
394-
position.line === 24 ? createStubHoverProvider({}, delayTime)(position) : of(null),
396+
position.line === 24
397+
? createStubHoverProvider({}, delayTime)(position)
398+
: of({ isLoading: false, result: null }),
395399
getActions: position =>
396400
position.line === 24
397401
? createStubActionsProvider(['foo', 'bar'], delayTime)(position)
@@ -465,7 +469,10 @@ describe('Hoverifier', () => {
465469
hoverOverlayElements: of(null),
466470
hoverOverlayRerenders: EMPTY,
467471
// Only show on line 24, not line 25 (which is the 2nd click event below).
468-
getHover: position => (position.line === 24 ? createStubHoverProvider({})(position) : of(null)),
472+
getHover: position =>
473+
position.line === 24
474+
? createStubHoverProvider({})(position)
475+
: of({ isLoading: false, result: null }),
469476
getActions: position =>
470477
position.line === 24 ? createStubActionsProvider(['foo', 'bar'])(position) : of(null),
471478
pinningEnabled: true,

src/hoverifier.ts

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import {
1919
import {
2020
catchError,
2121
debounceTime,
22-
delay,
2322
distinctUntilChanged,
2423
filter,
2524
first,
@@ -33,7 +32,7 @@ import {
3332
} from 'rxjs/operators'
3433
import { Key } from 'ts-key-enum'
3534
import { asError, ErrorLike, isErrorLike } from './errors'
36-
import { elementOverlaps, scrollIntoCenterIfNeeded } from './helpers'
35+
import { elementOverlaps, scrollIntoCenterIfNeeded, toMaybeLoadingProviderResult } from './helpers'
3736
import { calculateOverlayPosition } from './overlay_position'
3837
import { DiffPart, PositionEvent, SupportedMouseEvent } from './positions'
3938
import { createObservableStateContainer } from './state'
@@ -45,7 +44,8 @@ import {
4544
getTokenAtPosition,
4645
HoveredToken,
4746
} from './token_position'
48-
import { HoverAttachment, HoverOverlayProps, isPosition, LineOrPositionOrRange, LOADING } from './types'
47+
import { HoverAttachment, HoverOverlayProps, isPosition, LineOrPositionOrRange } from './types'
48+
import { emitLoading, MaybeLoadingResult, LOADING } from './loading'
4949

5050
export { HoveredToken }
5151

@@ -335,12 +335,17 @@ export const TOOLTIP_DISPLAY_DELAY = 100
335335
export const MOUSEOVER_DELAY = 50
336336

337337
/**
338+
* Function that returns a Subscribable or PromiseLike of the hover result to be shown.
339+
* If a Subscribable is returned, it may emit more than once to update the content,
340+
* and must indicate when it starts and stopped loading new content.
341+
* It should emit a `null` result if the token has no hover content (e.g. whitespace, punctuation).
342+
*
338343
* @template C Extra context for the hovered token.
339344
* @template D The type of the hover content data.
340345
*/
341346
export type HoverProvider<C extends object, D> = (
342347
position: HoveredToken & C
343-
) => SubscribableOrPromise<(HoverAttachment & D) | null>
348+
) => Subscribable<MaybeLoadingResult<(HoverAttachment & D) | null>> | PromiseLike<(HoverAttachment & D) | null>
344349

345350
/**
346351
* @template C Extra context for the hovered token.
@@ -716,21 +721,10 @@ export function createHoverifier<C extends object, D, A>({
716721
return of({ hoverOrError: null, position: undefined, part: undefined, codeViewId, ...rest })
717722
}
718723
// Get the hover for that position
719-
const hover = from(getHover(position)).pipe(
720-
catchError((error): [ErrorLike] => [asError(error)]),
721-
share()
722-
)
723-
// 1. Reset the hover content, so no old hover content is displayed at the new position while getting
724-
// 2. Show a loader if the hover hasn't returned after 100ms
725-
// 3. Show the hover once it returned
726-
return merge([undefined], of(LOADING).pipe(delay(LOADER_DELAY), takeUntil(hover)), hover).pipe(
727-
map(hoverOrError => ({
728-
...rest,
729-
codeViewId,
730-
position,
731-
hoverOrError,
732-
part: position.part,
733-
})),
724+
return toMaybeLoadingProviderResult(getHover(position)).pipe(
725+
catchError((error): [MaybeLoadingResult<ErrorLike>] => [{ isLoading: false, result: asError(error) }]),
726+
emitLoading<(HoverAttachment & D) | ErrorLike, null>(LOADER_DELAY, null),
727+
map(hoverOrError => ({ ...rest, codeViewId, position, hoverOrError, part: position.part })),
734728
// Do not emit anything after the code view this action came from got unhoverified
735729
takeUntil(allUnhoverifies.pipe(filter(unhoverifiedCodeViewId => unhoverifiedCodeViewId === codeViewId)))
736730
)
@@ -852,15 +846,15 @@ export function createHoverifier<C extends object, D, A>({
852846
if (pinningEnabled) {
853847
// DEFERRED HOVER OVERLAY PINNING
854848
// If the new position came from a click or the URL,
855-
// if either the hover or the definition turn out non-empty, pin the tooltip.
849+
// and either the hover or the definition turn out non-empty, pin the tooltip.
856850
// If they both turn out empty, unpin it so we don't end up with an invisible tooltip.
857851
//
858852
// zip together the corresponding hover and definition
859853
subscription.add(
860-
combineLatest(
854+
combineLatest([
861855
zip(hoverObservables, actionObservables),
862-
resolvedPositionEvents.pipe(map(({ eventType }) => eventType))
863-
)
856+
resolvedPositionEvents.pipe(map(({ eventType }) => eventType)),
857+
])
864858
.pipe(
865859
switchMap(([[hoverObservable, actionObservable], eventType]) => {
866860
// If the position was triggered by a mouseover, never pin
@@ -870,7 +864,7 @@ export function createHoverifier<C extends object, D, A>({
870864
// combine the latest values for them, so we have access to both values
871865
// and can reevaluate our pinning decision whenever one of the two updates,
872866
// independent of the order in which they emit
873-
return combineLatest(hoverObservable, actionObservable).pipe(
867+
return combineLatest([hoverObservable, actionObservable]).pipe(
874868
map(([{ hoverOrError }, actionsOrError]) =>
875869
// In the time between the click/jump and the loader being displayed,
876870
// pin the hover overlay so mouseover events get ignored

src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
export * from './hoverifier'
22
export * from './positions'
33
export { HoverOverlayProps } from './types'
4+
export * from './loading'

src/loading.test.ts

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
import { TestScheduler } from 'rxjs/testing'
2+
import { emitLoading, LOADING, MaybeLoadingResult } from './loading'
3+
4+
const deepStrictEqual = chai.assert.deepStrictEqual.bind(chai.assert)
5+
6+
const inputAlphabet: Record<'l' | 'e' | 'i' | 'r', MaybeLoadingResult<number | null>> = {
7+
// loading
8+
l: { isLoading: true, result: null },
9+
// empty
10+
e: { isLoading: false, result: null },
11+
// intermediate result
12+
i: { isLoading: true, result: 1 },
13+
// result
14+
r: { isLoading: false, result: 2 },
15+
}
16+
17+
const outputAlphabet = {
18+
// undefined
19+
u: undefined,
20+
// empty
21+
e: null,
22+
// loading
23+
l: LOADING,
24+
// intermediate result
25+
i: inputAlphabet.i.result,
26+
// result
27+
r: inputAlphabet.r.result,
28+
}
29+
30+
describe('emitLoading()', () => {
31+
it('emits an empty result if the source emits an empty result before the loader delay', () => {
32+
const scheduler = new TestScheduler(deepStrictEqual)
33+
scheduler.run(({ cold, expectObservable }) => {
34+
const source = cold('l 10ms e', inputAlphabet)
35+
expectObservable(source.pipe(emitLoading(300, null))).toBe('u 10ms e', outputAlphabet)
36+
})
37+
})
38+
it('emits a loader if the source has not emitted after the loader delay', () => {
39+
const scheduler = new TestScheduler(deepStrictEqual)
40+
scheduler.run(({ cold, expectObservable }) => {
41+
const source = cold('400ms r', inputAlphabet)
42+
expectObservable(source.pipe(emitLoading(300, null))).toBe('u 299ms l 99ms r', outputAlphabet)
43+
})
44+
})
45+
it('emits an empty result if the source completes without a result', () => {
46+
const scheduler = new TestScheduler(deepStrictEqual)
47+
scheduler.run(({ cold, expectObservable }) => {
48+
const source = cold('400ms |', inputAlphabet)
49+
expectObservable(source.pipe(emitLoading(300, null))).toBe('u 299ms l 99ms (e|)', outputAlphabet)
50+
})
51+
})
52+
it('emits an empty result if the source completes without a result before the loader delay', () => {
53+
const scheduler = new TestScheduler(deepStrictEqual)
54+
scheduler.run(({ cold, expectObservable }) => {
55+
const source = cold('10ms |', inputAlphabet)
56+
expectObservable(source.pipe(emitLoading(300, null))).toBe('u 9ms (e|)', outputAlphabet)
57+
})
58+
})
59+
it('emits an empty result if the source completes after an empty loading result', () => {
60+
const scheduler = new TestScheduler(deepStrictEqual)
61+
scheduler.run(({ cold, expectObservable }) => {
62+
const source = cold('l 400ms |', inputAlphabet)
63+
expectObservable(source.pipe(emitLoading(300, null))).toBe('u 299ms l 100ms (e|)', outputAlphabet)
64+
})
65+
})
66+
it('emits the last result if the source completes after an intermediate non-empty loading result', () => {
67+
const scheduler = new TestScheduler(deepStrictEqual)
68+
scheduler.run(({ cold, expectObservable }) => {
69+
const source = cold('-i-|', inputAlphabet)
70+
expectObservable(source.pipe(emitLoading(300, null))).toBe('ui-(i|)', outputAlphabet)
71+
})
72+
})
73+
it('errors if the source errors', () => {
74+
const scheduler = new TestScheduler(deepStrictEqual)
75+
scheduler.run(({ cold, expectObservable }) => {
76+
const error = new Error('test')
77+
const source = cold('10ms #', inputAlphabet, error)
78+
expectObservable(source.pipe(emitLoading(300, null))).toBe('u 9ms #', outputAlphabet, error)
79+
})
80+
})
81+
it('emits a loader if the source has not emitted a result after the loader delay', () => {
82+
const scheduler = new TestScheduler(deepStrictEqual)
83+
scheduler.run(({ cold, expectObservable }) => {
84+
const source = cold('l 400ms r', inputAlphabet)
85+
expectObservable(source.pipe(emitLoading(300, null))).toBe('u 299ms l 100ms r', outputAlphabet)
86+
})
87+
})
88+
it('emits a loader if the source first emits an empty result, but then starts loading again', () => {
89+
const scheduler = new TestScheduler(deepStrictEqual)
90+
scheduler.run(({ cold, expectObservable }) => {
91+
const source = cold('e 10ms l 400ms r', inputAlphabet)
92+
expectObservable(source.pipe(emitLoading(300, null))).toBe('(ue) 296ms l 111ms r', outputAlphabet)
93+
})
94+
})
95+
it('emits a loader if the source first emits an empty result, but then starts loading again after the loader delay already passed', () => {
96+
const scheduler = new TestScheduler(deepStrictEqual)
97+
scheduler.run(({ cold, expectObservable }) => {
98+
const source = cold('e 400ms l 400ms r', inputAlphabet)
99+
expectObservable(source.pipe(emitLoading(300, null))).toBe('(ue) 397ms l 400ms r', outputAlphabet)
100+
})
101+
})
102+
it('hides the loader when the source emits an empty result', () => {
103+
const scheduler = new TestScheduler(deepStrictEqual)
104+
scheduler.run(({ cold, expectObservable }) => {
105+
const source = cold('l 400ms e', inputAlphabet)
106+
expectObservable(source.pipe(emitLoading(300, null))).toBe('u 299ms l 100ms e', outputAlphabet)
107+
})
108+
})
109+
it('emits intermediate results before the loader delay', () => {
110+
const scheduler = new TestScheduler(deepStrictEqual)
111+
scheduler.run(({ cold, expectObservable }) => {
112+
const source = cold('l 10ms i 10ms r', inputAlphabet)
113+
expectObservable(source.pipe(emitLoading(300, null))).toBe('u 10ms i 10ms r', outputAlphabet)
114+
})
115+
})
116+
it('emits intermediate results after the loader delay and showing a loader', () => {
117+
const scheduler = new TestScheduler(deepStrictEqual)
118+
scheduler.run(({ cold, expectObservable }) => {
119+
const source = cold('l 400ms i 10ms r', inputAlphabet)
120+
expectObservable(source.pipe(emitLoading(300, null))).toBe('u 299ms l 100ms i 10ms r', outputAlphabet)
121+
})
122+
})
123+
})

src/loading.ts

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
import { OperatorFunction, merge, combineLatest, of } from 'rxjs'
2+
import { share, startWith, map, filter, mapTo, delay, endWith, scan, takeUntil, last } from 'rxjs/operators'
3+
import { isEqual } from 'lodash'
4+
5+
export const LOADING = 'loading' as const
6+
7+
/**
8+
* An emission from a result provider.
9+
*
10+
* @template T The type of the result. Should typically include an empty value, or even an error type.
11+
*/
12+
export interface MaybeLoadingResult<T> {
13+
/**
14+
* Whether the result provider is currently getting a new result.
15+
*/
16+
isLoading: boolean
17+
18+
/**
19+
* The latest result.
20+
*/
21+
result: T
22+
}
23+
24+
/**
25+
* Maps a stream of MaybeLoadingResult (which contains both results and loading states) to a stream of clear
26+
* instructions on when to show a loader, results or nothing.
27+
*
28+
* @param loaderDelay The delay, in milliseconds, after which a loader should be shown if no results have been emitted.
29+
* @param emptyResultValue The value that represents the absence of results. This will be emitted, and also deep-compared to with `isEqual()`. Example: `null`, `[]`
30+
*
31+
* @template TResult The type of the provider result (without `TEmpty`).
32+
* @template TEmpty The type of the empty value, e.g. `null` or `[]`.
33+
*/
34+
export const emitLoading = <TResult, TEmpty>(
35+
loaderDelay: number,
36+
emptyResultValue: TEmpty
37+
): OperatorFunction<MaybeLoadingResult<TResult | TEmpty>, TResult | TEmpty | typeof LOADING | undefined> => source => {
38+
const sharedSource = source.pipe(
39+
// Prevent a loading indicator to be shown forever if the source completes without a result.
40+
endWith<Partial<MaybeLoadingResult<TResult | TEmpty>>>({ isLoading: false }),
41+
scan<Partial<MaybeLoadingResult<TResult | TEmpty>>, MaybeLoadingResult<TResult | TEmpty>>(
42+
(previous, current) => ({ ...previous, ...current }),
43+
{ isLoading: true, result: emptyResultValue }
44+
),
45+
share()
46+
)
47+
return merge(
48+
// `undefined` is used here as opposed to `emptyResultValue` to distinguish between "no result" and the time
49+
// between invocation and when a loader is shown.
50+
// See for example "DEFERRED HOVER OVERLAY PINNING" in hoverifier.ts
51+
[undefined],
52+
// Show a loader if the provider is loading, has no result yet and hasn't emitted after LOADER_DELAY.
53+
// combineLatest() is used here to block on the loader delay.
54+
combineLatest([
55+
sharedSource.pipe(
56+
// Consider the provider loading initially.
57+
startWith({ isLoading: true, result: emptyResultValue })
58+
),
59+
// Make sure LOADER_DELAY has passed since this token has been hovered
60+
// (no matter if the source has emitted already)
61+
of(null).pipe(
62+
delay(loaderDelay),
63+
// Stop and ignore the timer when the source Observable completes
64+
takeUntil(sharedSource.pipe(last(null, null)))
65+
),
66+
]).pipe(
67+
// Show the loader when the provider is loading and has no result yet
68+
filter(([{ isLoading, result }]) => isLoading && isEqual(result, emptyResultValue)),
69+
mapTo(LOADING)
70+
),
71+
// Show the provider results (and no more loader) once the source emitted the first result or is no longer loading.
72+
sharedSource.pipe(
73+
filter(({ isLoading, result }) => !isLoading || !isEqual(result, emptyResultValue)),
74+
map(({ result }) => result)
75+
)
76+
)
77+
}

src/testutils/fixtures.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { delay } from 'rxjs/operators'
33

44
import { ActionsProvider, HoverProvider } from '../hoverifier'
55
import { HoverAttachment } from '../types'
6+
import { MaybeLoadingResult } from '../loading'
67

78
/**
89
* Create a stubbed HoverAttachment object.
@@ -28,7 +29,10 @@ export function createStubHoverProvider(
2829
hover: Partial<HoverAttachment> = {},
2930
delayTime?: number
3031
): HoverProvider<{}, {}> {
31-
return () => of(createHoverAttachment(hover)).pipe(delay(delayTime ?? 0))
32+
return () =>
33+
of<MaybeLoadingResult<{}>>({ isLoading: false, result: createHoverAttachment(hover) }).pipe(
34+
delay(delayTime ?? 0)
35+
)
3236
}
3337

3438
/**

0 commit comments

Comments
 (0)