Conversation
koesie10
force-pushed
the
koesie10/streaming-comparison
branch
from
571f689 to
a69ef15
Compare
This avoids a bug where comparisons could potentially overlap if the user opens a new comparison while the previous one has not yet finished loading.
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
A handful of simplifications, and a question.
Looks good, but I have not ran this myself.
| if (result.kind === "raw") { | ||
| return { | ||
| ...result, | ||
| from: result.from.slice(start, end), | ||
| to: result.to.slice(start, end), | ||
| }; | ||
| } | ||
|
|
||
| if (result.kind === "interpreted") { | ||
| return { | ||
| ...result, | ||
| from: result.from.slice(start, end), | ||
| to: result.to.slice(start, end), | ||
| }; | ||
| } |
There was a problem hiding this comment.
It looks like the bodies of these if statements are the same. Should you combine them?
There was a problem hiding this comment.
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 .
| if (msg.result.kind !== "raw") { | ||
| throw new Error( | ||
| "Streaming comparison: expected raw results, got interpreted results", | ||
| ); |
There was a problem hiding this comment.
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.
There shouldn't be: 510a269 adds a check for that race condition by adding an id to every message so that if a different setup message is received, it will ignore all future messages from the previous stream.
| result = { | ||
| ...prev.result, | ||
| from: [...prev.result.from, ...msg.result.from], | ||
| to: [...prev.result.to, ...msg.result.to], | ||
| }; |
There was a problem hiding this comment.
It looks like this assignment is the same as the one above. Can you merge the cases?
There was a problem hiding this comment.
This is split out for the same reason as above: we get a TypeScript error if we don't have this.
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
Given the limitations of the type system, I think this is fine. I'm sure there's some type wizardry we can do to make this cleaner, but it's really not worth the effort.
| if (result.kind === "raw") { | ||
| return { | ||
| ...result, | ||
| from: result.from.slice(start, end), | ||
| to: result.to.slice(start, end), | ||
| }; | ||
| } | ||
|
|
||
| if (result.kind === "interpreted") { | ||
| return { | ||
| ...result, | ||
| from: result.from.slice(start, end), | ||
| to: result.to.slice(start, end), | ||
| }; | ||
| } |
2 participants
This implements streaming for the compare view to avoid a crash when the total JSON.stringified size of the comparison is larger than 1GB (i.e. the Node.js string limit). It does this as follows:
toand at most 1,000fromresults in 1 message (i.e. 2,000 actual results).The 1,000 results per message is just a safe estimate where we should never run into the string limit. However, I can't really test this.