-
Notifications
You must be signed in to change notification settings - Fork 420
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
Workspace create file workaround #2019
Conversation
Adds a workaround for creating empty new files on the workspace side when a code action (such as generate type in a new file) is run. We need to add the document to the workspace when applying the code action, so that future updates aren't added to a transient file. However, this then triggers clients to send a new file notification. This can race with the actual content application: in fact, vscode seems to _always_ send the update buffer request before the create event is sent, even if the create and update changes are applied on the vscode side as separate, sequential workspace edits. To work around this, we detect when a file create is sent, the disk read has no content, and the workspace already has a document with real content in it.
// such as RunCodeAction. Unfortunately, previous attempts to have this fully controlled by the vscode | ||
// client (such that it sent both create event and then updated existing text) wasn't successful: | ||
// vscode seems to always trigger an update buffer event before triggering the create event. | ||
if ((await document.GetTextAsync()).Length > 0 && isCreate && string.IsNullOrEmpty(buffer)) |
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 would be slightly more efficient to check await document.GetTextAsync()).Length
last, because the other two conditions may short circuit and there will be no need to fetch the document text anymore
Thanks! I tested this locally and as we discussed in Slack, it is hacky but it indeed solves the issue 😄 update buffer -> file created event (with empty read from disk) -> file changed event upon save (with proper content) meant the file was only usable after save. In reality, the initial update buffer already puts the workspace in the correct state because the file was added to workspace upon code action execution. Now we have: |
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.
LGTM, in order to not block on this, I will merge this and we can swap the if order separately
edit: the cake tests failed.. let's see
- Remove CakeTextLoader as it will be triggered on 'document.GetTextAsync()' in BufferManader. This would force contents to be reloaded from disk and cached. Just updating buffer via UpdateBuffer requests seems like the safest approach here.
I've fixed the Cake side. I removed Honestly, I can't remember the initial reason for creating the |
Adds a workaround for creating empty new files on the workspace side when a code action (such as generate type in a new file) is run. We need to add the document to the workspace when applying the code action, so that future updates aren't added to a transient file. However, this then triggers clients to send a new file notification. This can race with the actual content application: in fact, vscode seems to always send the update buffer request before the create event is sent, even if the create and update changes are applied on the vscode side as separate, sequential workspace edits. To work around this, we detect when a file create is sent, the disk read has no content, and the workspace already has a document with real content in it.