-
Notifications
You must be signed in to change notification settings - Fork 202
Implement streaming for compare view #3771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,13 @@ | ||
import { useState, useEffect } from "react"; | ||
import { useState, useEffect, useRef } from "react"; | ||
import { styled } from "styled-components"; | ||
|
||
import type { | ||
ToCompareViewMessage, | ||
SetComparisonsMessage, | ||
SetComparisonQueryInfoMessage, | ||
UserSettings, | ||
StreamingComparisonSetupMessage, | ||
QueryCompareResult, | ||
} from "../../common/interface-types"; | ||
import { DEFAULT_USER_SETTINGS } from "../../common/interface-types"; | ||
import CompareSelector from "./CompareSelector"; | ||
|
@@ -37,6 +39,12 @@ export function Compare(_: Record<string, never>): React.JSX.Element { | |
DEFAULT_USER_SETTINGS, | ||
); | ||
|
||
// This is a ref because we don't need to re-render when we get a new streaming comparison message | ||
// and we don't want to change the listener every time we get a new message | ||
const streamingComparisonRef = useRef<StreamingComparisonSetupMessage | null>( | ||
null, | ||
); | ||
|
||
const message = comparison?.message || "Empty comparison"; | ||
const hasRows = | ||
comparison?.result && | ||
|
@@ -53,6 +61,87 @@ export function Compare(_: Record<string, never>): React.JSX.Element { | |
case "setComparisons": | ||
setComparison(msg); | ||
break; | ||
case "streamingComparisonSetup": | ||
setComparison(null); | ||
streamingComparisonRef.current = msg; | ||
break; | ||
case "streamingComparisonAddResults": { | ||
const prev = streamingComparisonRef.current; | ||
if (prev === null) { | ||
console.warn( | ||
'Received "streamingComparisonAddResults" before "streamingComparisonSetup"', | ||
); | ||
break; | ||
} | ||
|
||
if (prev.id !== msg.id) { | ||
console.warn( | ||
'Received "streamingComparisonAddResults" with different id, ignoring', | ||
); | ||
break; | ||
} | ||
|
||
let result: QueryCompareResult; | ||
switch (prev.result.kind) { | ||
case "raw": | ||
if (msg.result.kind !== "raw") { | ||
throw new Error( | ||
"Streaming comparison: expected raw results, got interpreted results", | ||
); | ||
} | ||
Comment on lines
+87
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any danger of getting here if you switch in the middle of rendering a very large comparison result? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There shouldn't be: 510a269 adds a check for that race condition by adding an |
||
|
||
result = { | ||
...prev.result, | ||
from: [...prev.result.from, ...msg.result.from], | ||
to: [...prev.result.to, ...msg.result.to], | ||
}; | ||
break; | ||
case "interpreted": | ||
if (msg.result.kind !== "interpreted") { | ||
throw new Error( | ||
"Streaming comparison: expected interpreted results, got raw results", | ||
); | ||
} | ||
|
||
result = { | ||
...prev.result, | ||
from: [...prev.result.from, ...msg.result.from], | ||
to: [...prev.result.to, ...msg.result.to], | ||
}; | ||
break; | ||
Comment on lines
+106
to
+110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this assignment is the same as the one above. Can you merge the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is split out for the same reason as above: we get a TypeScript error if we don't have this. |
||
default: | ||
throw new Error("Unexpected comparison result kind"); | ||
} | ||
|
||
streamingComparisonRef.current = { | ||
...prev, | ||
result, | ||
}; | ||
|
||
break; | ||
} | ||
case "streamingComparisonComplete": | ||
if (streamingComparisonRef.current === null) { | ||
console.warn( | ||
'Received "streamingComparisonComplete" before "streamingComparisonSetup"', | ||
); | ||
setComparison(null); | ||
break; | ||
} | ||
|
||
if (streamingComparisonRef.current.id !== msg.id) { | ||
console.warn( | ||
'Received "streamingComparisonComplete" with different id, ignoring', | ||
); | ||
break; | ||
} | ||
|
||
setComparison({ | ||
...streamingComparisonRef.current, | ||
t: "setComparisons", | ||
}); | ||
streamingComparisonRef.current = null; | ||
break; | ||
case "setUserSettings": | ||
setUserSettings(msg.userSettings); | ||
break; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the bodies of these if statements are the same. Should you combine them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately that's not possible in this case because TypeScript can't determine that automatically from these overlapping types:
Returned expression type {kind: "raw" | "interpreted", from: Row[] | Result[], to: Row[] | Result[]} is not assignable to type QueryCompareResult
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunate.