Allow extensions to handle saves of unmodified files

[DanTup]

TL;DR - please could we have an on-save event that fires under the same situations that VS Code invokes the on-save formatter.

---

There's a difference between how VS Code handles format-on-save versus what extensions are able to do with their own actions on-save, because the save handlers don't fire if the user explicitly saves a file that has been auto-saved, yet VS Code does run the formatter.

For example:

• enable autoSave: afterDelay
• edit a file - and note that VS Code will suppress the call to the formatter when it auto saves (this makes sense)
• press Save/Cmd+S and notice that the formatter runs

If you want to implement something in an extension (let's say when the user modifies pubspec.yaml, you want to run pub get to fetch the packages, but you don't want to do it on auto-saves as the file may be incomplete), it fails - because the last step above (pressing Save/Cmd+S) does not invoke onWillSave/onDidSave.

This means we have to have clumsy workarounds - currently for Dart we start a 10second timer after an auto-save, and keep pushing it back on subsequent saves. This means we will eventually fetch packages when we think the user has finished modifying the file, but it's kinda hit-and-miss (sometimes it'll fire while they've paused to think, and sometimes they have to wait up to 10 seconds for it to do what they need).

[DanTup]

Another place this would help - "Hot Reload on Save". Currently if you have auto-save turned on, we will hot reload on every auto-save. If we do it only for manual saves, then a user who has auto-save on will have to hit save before the auto-save delay in order for it to work, otherwise they will get no Hot Reload on Cmd+S.

[jrieken]

it fails - because the last step above (pressing Save/Cmd+S) does not invoke onWillSave/

No the onWillSave will always fire, check this. @DanTup please provide a minimal sample extension that demonstrates what you are claiming

[DanTup]

Hmm, that's not the behaviour I'm seeing. I've got handlers that print out in these two events:

context.subscriptions.push(workspace.onDidSaveTextDocument((td) => {
	console.log(`did save ${fsPath(td.uri)}`);
}));
context.subscriptions.push(workspace.onWillSaveTextDocument((e) => {
	console.log(`will save ${fsPath(e.document.uri)}: ${TextDocumentSaveReason[e.reason]}`);
}));

And then in my document formatter I also log:

public async provideDocumentFormattingEdits(document: TextDocument, options: FormattingOptions, token: CancellationToken): Promise<TextEdit[]> {
		console.log(`Running document formatter for ${fsPath(document.uri)}`);

Here's what I see logged:

// Type something then pause
will save /Users/dantup/Dev/Google/flutter_web/examples/spinning_square/lib/main.dart: AfterDelay
did save /Users/dantup/Dev/Google/flutter_web/examples/spinning_square/lib/main.dart

// Wait 10 seconds, then press Cmd+S
Running document formatter for /Users/dantup/Dev/Google/flutter_web/examples/spinning_square/lib/main.dart

Neither onWillSave of onDidSave fired when I pressed Cmd+S at the end (but the formatter ran).

[DanTup]

Oh, hangon - I see what's happening. If the formatter throws, then the onWillSave does not fire. Is this intended, or should it run even if a formatter throws?

We let our formatter throw if the file is not syntactically correct - I don't think there's a good reason for this, so I can change it - however I don't know if it's expected that a throwing formatter will stop the other handlers firing.

[DanTup]

That said, there's still an issue here - we can't start anything in onWillSave because the document hasn't saved yet. We need to run after the file is committed. Is there a good way to do that other than just setTimeout with some small delay we think is long enough?

Thanks!

[jrieken]

If the formatter throws, then the onWillSave does not fire. Is this intended, or should it run even if a formatter throws?

Not sure, it seems to have been like that ever since. Tho, I think the formatter shouldn't throw in the presence of syntax errors but just not do anything.

we can't start anything in onWillSave because the document hasn't saved yet

Well, if the document doesn't change, e.g if the document doesn't get modified during will-save, then we don't save it and therefore no did-save fires. TBH I think that behaviour is sound and I am not sure if listen for saving is good for your use-case. Why don't you listen on modification and check if a file is valid before doing any follow-up work?

[DanTup]

Tho, I think the formatter shouldn't throw in the presence of syntax errors but just not do anything.

Yeah, I agree with that. I think our handling is bad. But still, I think if there was a legit error during formatting though, I wouldn't expect my willSave handler not to fire. I think maybe that should be changed (or at least, the behaviour documented - especially if a formatter from another extension could cause it - I don't know if that's the case).

Well, if the document doesn't change, e.g if the document doesn't get modified during will-save, then we don't save it and therefore no did-save fires. TBH I think that behaviour is sound

In some ways it sounds reasonable, but the naming of onWillSave also sounds like it will save :)

Why don't you listen on modification and check if a file is valid before doing any follow-up work?

It would still feel really weird performing either of these actions on auto-save, even if we abort then the file isn't valid. For ex., if in my dependencies I'm adding:

my_package: 1.2.3

If I pause briefly after typing 1.2, the file is still valid, and we'll start fetching the packages needlessly, then when I type .3 we'll cancel it and start it again. If you're typing at just the right (wrong) speed, we'll be spawning and killing processes. Really we want an explicit user action to trigger it (Cmd+S is most natural).

Hot Reload is similar - if I've enabled hot-reload-on-save and also VS Code's autoSave, I probably don't want auto-saves to perform a hot reload (because I'm likely to pause while typing code - which may be valid - to think, and although hot reload is fast, it's distracting and I might have wanted to keep the device in the state it was in while I type my changes).

In my head, it seems like an obvious event to have, but when I try to describe it sounds rather specific. I guess it it's most applicable to when you want to do something on-save, but that work is expensive or destructive, so you want only user-initiated events.

How about if onWillSave handlers could register a callback to run after the save (with an option for whether it should run even if there are no changes)? That could also improve the weird niggle that if you want to do something in onDidSave but want to know the reason for the save, you have to also hook onWillSave and then stash the reason in a variable. I think the idea of being able to do something after a save but also knowing whether it was manual or auto is reasonable? :)

I see there's already a waitUntil on TextDocumentWillSaveEvent, maybe there could be something similar?

context.subscriptions.push(vs.workspace.onWillSaveTextDocument((e) => {
	e.afterSave(() => {
		if (e.reason === vs.TextDocumentSaveReason.Manual) {
			// Do something here
		}
	}); // + some flag to run even if there were no changes made?
}));

[jrieken]

If I pause briefly after typing 1.2, the file is still valid, and we'll start fetching the packages needlessly, then when I type .3 we'll cancel it and start it again. If you're typing at just the right (wrong) speed, we'll be spawning and killing processes.

You can never avoid that with async operations. What would you do if the user presses Cmd+S repeatedly? With or without changes? You will always debounce and cancel/discard work. In your sample, type 1.2, save, type .3 without save. I think what matters here is that the operation is async, not how/when you trigger it.

For less lightweight operations, esp. those that sync from state kept in files, you need onWill/DidSave (file watching might even be better) but I still think everything is there. Why would you kick off hot reload when the file wasn't changed (e.g. no didSave-event)? The state on disk is as before, no follow up work. What will hot reload change when nothing in the file has changed?

It seems what you want is to piggyback on the cmd+s shortcut, independent of save or not. Kinda like listening to a command being executed so that another things can run...

[DanTup]

You can never avoid that with async operations

I can't avoid having to cancel and re-run, sure - but that doesn't mean I have to keep firing while the user is typing - that's a bad experience (esp. when there's a visible progress indicator on screen while the process runs).

As a I user, I want the operation to happen when I press Cmd+S. Always. Never on auto-save, but always on Cmd+S. Cmd+S is my shortcut for "save this file (if it needs it) and run anything that is appropriate to do when I save this file". Pressing Cmd+S on an undirty file is sort-of a way of saying "I want you to do the on-save things, even though this file isn't modified". That VS code already runs the formatter in this case shows that there are good reasons for it :-)

Why would you kick off hot reload when the file wasn't changed (e.g. no didSave-event)? The state on disk is as before, no follow up work. What will hot reload change when nothing in the file has changed?

It's exactly the same as formatting. Why doesn't VS Code auto-format on an auto-save? Because it'd be (really!) annoying. Yet it does then format when I press Cmd+S even though there are no changes.

My action is basically the same - something that shouldn't be auto-triggered, only from a user-action - however it's important that it runs after the file is committed (unlike for formatting, which can run on the in-memory file buffer).

As a workaround today, we just debounce in the auto-save handler - however there's no good value to sleep for. We currently have it set for 10 seconds, but for auto-save users, that means this happens:

• User makes an edit
• VS code auto-saves - we start a 10 second timer
• User pressed Cmd+S - nothing happens because onDidSave didn't fire
• 10 seconds later, action runs

This is painful for the auto-save users. We can reduce the timer from 10s, but that makes it more annoying if the user is pausing while typing. There's not perfect value to set. Maybe we can hook onWillSave, see if it's manual, and there's an ongoing timer, then cancel the timer and run immediately, but it's starting to seem complicated/fragile.

It seems what you want is to piggyback on the cmd+s shortcut, independent of save or not. Kinda like listening to a command being executed so that another things can run...

Yeah, pretty much exactly this - but it's important that it always fires even when there was no real save, and that we don't have to keep track ourselves of what the save keybinding is if the user has changed it (or uses File -> Save, etc.). Again, this seems to be exactly what formatting does, except for it's ok to run pre-save because it doesn't use the file off disk for its actions.

There is also the code actions that run on save, but I've not tested when they fire, mainly because I don't think there's a good way for an extension to register them without contributing a setting that would overwrite the ones the user has configured themselves.

[jrieken]

Yet it does then format when I press Cmd+S even though there are no changes

Before invoking format we don't know that there are no changes. After we know and then we won't make changes on disk unless the file has been modified.

I am closing this as dupe #1431 (and/or #72345) because you aren't interested in saving but in press+s being pressed

[DanTup]

I am closing this as dupe #1431 (and/or #72345) because you aren't interested in saving but in press+s being pressed

Seems reasonable - but just to be clear, would this support things like File->Save too? Does that just fire the same save command?

Thanks!