API: Extra events like onPreSaveTextDocument?

[bpasero]

It would be nice to have some extra events I guess. The first one I can think of is onPreSaveTextDocument (or onSaveTextDocument). One that will be invoked right before the document gets saved.

Use Cases

The first one I can think of is my own plugin final-newline that will insert a final newline at the end of the file when the user saves the document. At the moment it is implemented like this: Cmd + s → Save → Insert newline (if necessary) → Save
That last save triggers a new iteration of my plugin which will detect a final newline is already present and will stop the execution. So my loop is triggered twice because the plugin is triggered after the document was saved instead of before.

I think there will be other file manipulation plugins that want to manipulate the file before it gets saved as well.

From Alex:

I agree some sort of onBeforeSave action is indeed needed, we just need more thinking regarding how to expose this without causing any data loss. If we block the save to wait on extensions, I can imagine it possible that the wait could be indefinite, resulting in data loss. Maybe a time ceiling of 10s, after which the file gets saved...

@bpasero What do you think?

[lukehoban]

The top requested feature for the Go extension is a format-on-save option (microsoft/vscode-go#14).

This really begs for being invoked on a pre-save event.

Without it, we have to do some sort of double-save with bookkeeping to make sure the first save doesn't re-trigger the post save event. The best I have managed is this: https://github.com/Microsoft/vscode-go/blob/formatonsave/src/goMain.ts#L99-L118. However, the double-save flashes the (unsaved) marker in the editor and I'm sure there are other issues with this approach.

It would be great to have a pre-save hook to use instead.

[bpasero]

We have a FILE_SAVING event but a) it is not exposed to extensions and b) it currently expects modifications to happen sync and not async, which would be a requirement when we talk from editor to extensions.

[jrieken]

fyi @dbaeumer

[dbaeumer]

@bpasero is this something we can work on. I would like to hook up auto fix support for ESLint and doing this before save would really make a good story.

[bpasero]

Yeah, stretch for April, otherwise May.

[mysticatea]

What is happening on this?

[jaime-olivares]

So, it was planned for April, and we are July. Is this already included in VSCode 1.3?

[mysticatea]

@bpasero @dbaeumer Is there plan to implement this? This is blocking the feature of auto-fixing on save for several extensions. 😕

[bpasero]

Thanks so much for your interest in this issue! It is currently assigned to the backlog. Every month we pick items from the backlog to plan for the current iteration. Please see https://github.com/Microsoft/vscode/wiki/Issue-Tracking#planning for more information.

Since we are a small team of developers, there is only so many feature requests and issues we can work on for one milestone. Nevertheless we always welcome pull requests and are happy to accept them if they meet our quality standards.

[mysticatea]

OK, I'm happy to investigate this in my summer vacation. Before it, I'd like to know, were there any concerns on this since this was dropped from the April/May plan?

[SamVerschueren]

but what I don't understand, is how will extensions be aware of others extensions that will be executed onSave ?

Not... That's the whole thing. It should be independent of other extensions. Like @jrieken said before, the thing you are doing is either (a) a bug on the GoFormat extension or (b) something uncommon to do.

[kube]

Ok, I understand that.
The thing is, from a user point of view, if I want to apply a pipeline of modifications in a certain order, like some function composition, doing:

editA . editB

is not the necessarily the same as

editB . editA

What I mean is that these edits are not necessarily commutative.

I understand that it's not the good way to permit extension authors to self-define their extension priority, but what happens if a user needs to specify it? (As in my case)

I'm pretty sure that I won't be the only one facing this problem once the onWillSaveTextDocument will be available for everyone.

[jrieken]

You can only control that when you make both edits yourself - otherwise there is no way to know what another extension is going to change. There is always a chance of having extensions that conflict with each other and ultimately its the user that decides by installing/uninstalling extensions.

On the happy side, note that work on 'format on save' (#12449) started and that the current sequence is 'trim trailing whitespace', 'run formatter', 'onWillSave-listeners'. That means you are lucky wrt gofmt but again there is no guarantee a listener won't undo the changes your extension just made.

[jrieken]

It could then sort them and invoke the fast-running pre-save actions first.

@SamVerschueren Not so sure anymore as this is a little unfair. There might be extensions that do honest time consuming work, like building an AST, shelling out to another process etc and we shouldn't punish them for that.

I am leaning towards something like this: Have a total budget for all listeners (the second mentioned above), monitor each listener and if they behave bad (throwing n-times, spending multiples of the overall budget) we 'uninstall' them and inform the user. It's some sort of natural selection of listeners :-)

[DanTup]

@SamVerschueren
Or a different strategy. Save the document to disk first (make sure nothing gets lost), and then try to apply all the pre-save events (best-effort) and save it again.

Saving twice could cause issues (or duplicate effort) for tools that watch the filesystem and do some processing when contents change? (Even though most things that watch the fs probably have some threshold, they're probably not set with "slow" language services in mind).

[SamVerschueren]

@DanTup Good point!

@jrieken Yes it was just something that came up in my mind that I wanted to share. You have a valid point there. Your solution would be nice though! Keeping track of extensions that misbehave and disable/uninstall them.

[jrieken]

@bpasero Thinking of adding source or reason property to the event. Is it possible to expose the different save reasons to the participants? Like explicit (cmd+s, menu item), focuson, window change, auto? Given we need that information in our participants it's likely consumers of this API need it as well

[bpasero]

@jrieken I think that is easy to add because a user can only have 1 auto save method enabled. If the participant checks for the isAutoSaved property, I think all the info is there:

• false: it can only be a manual save
• true: go to configuration service and ask for the auto save setting (or use the ITextFileService#getAutoSaveConfiguration() for convenience (https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/files/common/files.ts#L400)

[jrieken]

false: it can only be a manual save

Actually not. I see isAutoSaved as false when using save on focus out. Which I like btw because it makes a nice format on focus lost experience

[bpasero]

@jrieken ah yes, I can look into adding this info and then ping you how to expose it to the participant. it needs to be wired in from some places

[jrieken]

k

[bpasero]

@jrieken there is now a new SaveReason enum carried all the way to the participant (https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/files/common/editors/textFileEditorModel.ts#L453) if you wanna pick it up.

Technically there can still be saves happening via API and currently I treat those as manual saves to distinguish from the auto saves (after delay, after focus change, after window change). One examples of where a save can kick in is when you start a debug session and debug triggers a save of all dirty files first. I think it is fair in that case to run the participants because it is an explicit user interaction to start debug.

[jrieken]

That is how it looks

    /**
     * Represents reasons why a text document is saved.
     */
    export enum TextDocumentSaveReason {

        /**
         * Explicitly triggered, e.g. by the user pressing save or by an API call.
         */
        Explicit = 1,

        /**
         * Automatic after a delay.
         */
        Auto = 2,

        /**
         * When the editor lost focus.
         */
        FocusOut = 3
    }

    /**
     * An event that is fired when a [document](#TextDocument) will be saved.
     *
     * To make modifications to the document before it is being saved, call the
     * [`waitUntil`](#TextDocumentWillSaveEvent.waitUntil)-function with a thenable
     * that resolves to an array of [text edits](#TextEdit).
     */
    export interface TextDocumentWillSaveEvent {

        /**
         * The document that will be saved.
         */
        document: vscode.TextDocument;

        /**
         * The reason why save was triggered.
         */
        reason: TextDocumentSaveReason;

        /**
         * Allows to pause the event loop and to apply [pre-save-edits](#TextEdit).
         * Edits of subsequent calls to this function will be applied in order. The
         * edits will be *ignored* if concurrent modifications of the document happened.
         *
         * *Note:* This function can only be called during event dispatch and not
         * in an asynchronous manner:
         *
         * ```ts
         * workspace.onWillSaveTextDocument(event => {
            // async, will *throw* an error
            setTimeout(() => event.waitUntil(promise));

            // sync, OK
         *  event.waitUntil(promise);
         * })
         * ```
         *
         * @param thenable A thenable that resolves to [pre-save-edits](#TextEdit).
         */
        waitUntil(thenable: Thenable<vscode.TextEdit[]>): void;

        /**
         * Allows to pause the event loop until the provided thenable resolved.
         *
         * *Note:* This function can only be called during event dispatch.
         *
         * @param thenable A thenable that delays saving.
         */
        waitUntil(thenable: Thenable<any>): void;
    }

[SamVerschueren]

Great work guys! 🍻