Snapshot TextDocuments when delayOpenNotifications=true to avoid serving updated info to middleware/server#1755
Snapshot TextDocuments when delayOpenNotifications=true to avoid serving updated info to middleware/server#1755DanTup wants to merge 4 commits into
Conversation
…ing updated info to middleware/server When delayOpenNotifications is enabled and an open notification is delayed, we cannot use the live VS Code TextDocument to pass to middleware or to the server because it may have updated version/content info. Instead, capture a snapshot of the document at the time the original notification would've been sent, and then when the notification will be sent, provide that snapshot to the middleware and build the server parameters from it. Fixes microsoft#1695
DanTup
force-pushed
the
delay-open-notifications
branch
from
3b58e46 to
d12ba08
Compare
| } No newline at end of file | ||
| } | ||
|
|
||
| class TextDocumentSnapshot implements TextDocument { |
There was a problem hiding this comment.
@dbaeumer the implementation here was written by GPT-5.4 with some tidying up by me. I don't like that so much is repeated here from VS Code (and I haven't tried to verify the behaviour matches perfectly, because I'm hoping you might have a better idea :-))
I did add 'vscode-languageserver-textdocument' but it only provided a small number of the methods we needed.
I also noticed there is a very similar class here, but it uses some additional types from VS Code so I couldn't just lift it:
I wonder if it'd be better adding a good implementation of this to vscode-languageserver-textdocument and then using it both in Copilot and here?
|
|
||
| class TextDocumentSnapshot implements TextDocument { | ||
|
|
||
| private static readonly DefaultWordRegExp: RegExp = /(-?\d*\.\d\w*)|([^\s`~!@#%^&*()=+[\]{}\\|;:'",.<>/?-]+)/g; |
There was a problem hiding this comment.
Question: did you copy this from somewhere?
There was a problem hiding this comment.
No, as noted above - most of the implementation here was written by GPT 5.4. Although TypeScript has a very similar class here:
It's possible that this was in its training (it uses DEFAULT_WORD_REGEXP imported from the VS Code source).
I don't like having all this code here, and since TS has a version too, but would really be nice if they could be shared in some way. This feels a bit fragile (and I have not fully reviewed the generated code here yet, because I was hoping you had a better idea 😄)
There was a problem hiding this comment.
I think this might also be incorrect depending on the language?
|
@DanTup nice work. |
|
@DanTup I am planning to do a new stable LSP release soon and I would like to include this PR since the implementation makes sense to me. Could you address the remaining issues I flaged? |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dbaeumer yes, I'll fix that - but do you have any concerns about the generated code or hard-coded regex noted above? |
|
Mo concerns about the code or the regexp. I only wanted to know where the RegExp is coming from. If copied from somehwere would be good to left a pointer |
|
Ok, sgtm as long as you're happy with that generated class. I do think it'd be nice to have a central "official" implementation (since TS is doing it too), but that could always be done in future. I realise I didn't add any test for the |
2 participants
When delayOpenNotifications is enabled and an open notification is delayed, we cannot use the live VS Code TextDocument to pass to middleware or to the server because it may have updated version/content info.
Instead, capture a snapshot of the document at the time the original notification would've been sent, and then when the notification will be sent, provide that snapshot to the middleware and build the server parameters from it.
Fixes #1695