Use previous responses for completionEntryDetails.
#9
Conversation
DanielRosenwasser
requested review from
andrewbranch,
gabritto,
MariaSolOs and
jakebailey
| if (actualEntry.name === replayEntry.name | ||
| && actualEntry.source === replayEntry.source | ||
| && actualEntry.exportName === replayEntry.exportName |
There was a problem hiding this comment.
How much of this is redundant with name and source?
There was a problem hiding this comment.
The name, source, and data composite is uniquely identifying, but not all of data is needed to disambiguate, probably. I’ve never heard of exportName. (name and source are enough 99.9% of the time, but I think there was something that needed data as a disambiguation, not just as an optimization.)
There was a problem hiding this comment.
Ugh, exportName is from data. That's a bug.
| @@ -283,9 +283,42 @@ async function main() { | |||
| } | |||
| }); | |||
|
|
|||
| /** @type {import("typescript/lib/protocol").CompletionInfoResponse | undefined} */ | |||
There was a problem hiding this comment.
IDK if we're already using it (have not expanded the diff), but we should change to use ts.server.protocol here given I eliminated /lib/protocol in 5.0.
|
|
||
| const actualEntries = lastCompletionsResponse.body.entries; | ||
| // Typically these will be sequential, but we might need to loop back around. | ||
| lastCompletionEntryDetailsIndex = findSuitableCompletionEntryDetailAndReplaceData(lastCompletionEntryDetailsIndex, actualEntries, replayEntry); |
There was a problem hiding this comment.
I wanted to avoid going definitely-quadratic, but this double pass feels bad and a map on every completions request might start to add up?
There was a problem hiding this comment.
If everything else in the replay is accurate, the indices should always be the same, right? Also, is detailsRequest.arguments.entryNames ever longer than 1? AFAIK, nobody uses the API to get more than one set of details at a time.
There was a problem hiding this comment.
I'm just trying to be robust on that.
If everything else in the replay is accurate, the indices should always be the same, right?
I thought so, but without the "loop around again" logic, it does fail on some repros. 😬
Similarly, we don't necessarily always find the same completion entry either!
There was a problem hiding this comment.
You could build a lookup table of name/source/data first, but it will probably be worse than what you’re doing now if entryNames is only 1 long.
Follow-up from microsoft/typescript-error-deltas#99, and currently a work-in-progress.
I'm not sure of specifics, but TypeScript's completion responses might contain something that is specific to that session, the program, the environment, etc.
This PR ensures that whenever the replay script runs into a
completionEntryDetailsrequest withdata, it tries to find a corresponding response from the previouscompletionInforequest.There are some cases where the script was not able to find a corresponding
CompletionEntry, which makes me a bit nervous about this prototype. I can't tell if that's a divergence between how I've installed something in two places, versus something wrong with the replay script. But I just want to get thoughts on the approach for now because it does feel like a hack.