Skip to content

Commit

Permalink
feat: Reject promise immediately if var not found (#1718)
Browse files Browse the repository at this point in the history
- Implements #1701 
- `fetchVariableDefinition` now throws an `Error` when a variable is not
found (instead of throwing `TimeoutError`)
- `TimeoutError` still used and will be thrown on an actual timeout
- Tests added to check for the correct error

---------

Co-authored-by: Mike Bender <mikebender@deephaven.io>
  • Loading branch information
wusteven815 and mofojed authored Jan 11, 2024
1 parent 51ebe1b commit 43d40bd
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 30 deletions.
36 changes: 9 additions & 27 deletions packages/jsapi-utils/src/ConnectionUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,38 +56,24 @@ it('finds the right definition if variable exists', async () => {
expect(mockRemoveListener).toHaveBeenCalled();
});

it('finds the definition in the second update if not after the first update', async () => {
it('throws a TimeoutError if finding the variable timed out', async () => {
const timeout = 1000;
const fetchPromise = fetchVariableDefinition(
connection,
testDefinition2.title
testDefinition2.title,
timeout
);

expect(mockSubscribeToFieldUpdates).toHaveBeenCalled();
expect(mockRemoveListener).not.toHaveBeenCalled();

const listener = mockSubscribeToFieldUpdates.mock.calls[0][0];

listener({
created: [testDefinition1],
updated: [],
removed: [],
});

expect(mockRemoveListener).not.toHaveBeenCalled();
jest.advanceTimersByTime(timeout + 2000);

listener({
created: [testDefinition2],
updated: [],
removed: [],
});

const result = await fetchPromise;

expect(result).toBe(testDefinition2);
await expect(fetchPromise).rejects.toThrow(TimeoutError);
expect(mockRemoveListener).toHaveBeenCalled();
});

it('throws a TimeoutError if variable not found', async () => {
it('throws an Error if variable not found', async () => {
const fetchPromise = fetchVariableDefinition(
connection,
testDefinition2.title
Expand All @@ -104,11 +90,7 @@ it('throws a TimeoutError if variable not found', async () => {
removed: [],
});

expect(mockRemoveListener).not.toHaveBeenCalled();

jest.runOnlyPendingTimers();

await expect(fetchPromise).rejects.toThrow(Error);
await expect(fetchPromise).rejects.not.toThrow(TimeoutError);
expect(mockRemoveListener).toHaveBeenCalled();

await expect(fetchPromise).rejects.toThrow(TimeoutError);
});
8 changes: 5 additions & 3 deletions packages/jsapi-utils/src/ConnectionUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export function fetchVariableDefinition(

const timeoutId = setTimeout(() => {
removeListener?.();
reject(new TimeoutError(`Variable ${name} not found`));
reject(new TimeoutError(`Timeout looking for variable ${name}`));
}, timeout);

/**
Expand All @@ -34,10 +34,12 @@ export function fetchVariableDefinition(
*/
function handleFieldUpdates(changes: VariableChanges): void {
const definition = changes.created.find(def => def.title === name);
clearTimeout(timeoutId);
removeListener?.();
if (definition != null) {
clearTimeout(timeoutId);
removeListener?.();
resolve(definition);
} else {
reject(new Error(`Variable ${name} not found`));
}
}

Expand Down

0 comments on commit 43d40bd

Please sign in to comment.