Skip to content

Remove didSave notification handler#1080

Merged
pfitzseb merged 3 commits intomasterfrom
sp/rm-didSave-notification-handler
Jun 9, 2022
Merged

Remove didSave notification handler#1080
pfitzseb merged 3 commits intomasterfrom
sp/rm-didSave-notification-handler

Conversation

@pfitzseb
Copy link
Copy Markdown
Member

No description provided.

@pfitzseb pfitzseb requested a review from davidanthoff May 10, 2022 10:12
@davidanthoff
Copy link
Copy Markdown
Member

Copying a comment I just left on Slack:

I think actually that we added the didSave handler exclusively to diagnose text sync bugs. The thinking was essentially that it would be a way to compare the entire doc document at a specified "sync" point, and if we don't get crash reports from that we could be very, very certain that our text sync implementation is correct, without trying to diagnose individual didChange workflows.

So I'm starting to wonder whether we really should disable it, or try to figure out what exactly is going on, because by our old logic, if we see crashes here, there is something wrong in our text sync implementation...

@pfitzseb
Copy link
Copy Markdown
Member Author

I'm not sure the premise is entirely correct -- I'll ask upstream whether there's any guarantee for getting didChange before didSave.

@davidanthoff
Copy link
Copy Markdown
Member

I'm not sure the premise is entirely correct

What speaks in favor of the premise, though, is that we haven't seen this crash at all for months, if not years, in our crash reporting, AFAIK.

@pfitzseb
Copy link
Copy Markdown
Member Author

True. The client side behaviour does appear to change with the new LSP release though (julia-vscode/julia-vscode#2886, in case you wanna give it a go).

@davidanthoff
Copy link
Copy Markdown
Member

Maybe we should report this as a bug in the new client side implementation?

@pfitzseb pfitzseb merged commit 4aef2c1 into master Jun 9, 2022
@davidanthoff davidanthoff deleted the sp/rm-didSave-notification-handler branch June 17, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants