Vscode: 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 (https://github.com/Microsoft/vscode-go/issues/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. :confused:

[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?

[DanTup]

Another vote for this - we can't support format-on-type in Dart Code currently because the analyser doesn't support partial formatting (I can't think of a clever way I could massage a full-document format in a way to make this work) so format-on-save would be the next best thing.

Bonus points if we can hook additional edits into this (eg. sorting imports) at the same time!

[jrieken]

@bpasero Is the save participant API fit for async operations? I have the following API in mind but it requires that I can participant to save in an async way

interface TextDocumentWillSaveEvent {
  document: TextDocument;
  pushEdits(edits: TextEdit[] | Thenable<TextEdit[]>): void;
}

onWillSaveTextDocument: Event<TextDocumentWillSaveEvent>

So the idea is that the event allows you to push text edits that will be applied to the document before saving. When multiple _will-save_-listeners exist edits will be applied in sequences like

1. `onWillSaveTextDocument` fires
2. will-save-listener A called - makes changes using `pushEdits`
3. `onDidChangeTextDocument` fires
4. will-save-listener B called - makes changes using `pushEdits`
5. `onDidChangeTextDocument` fires
6. save on latest state happens

[bpasero]

@jrieken the save participant today is sync (see https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/files/common/editors/textFileEditorModel.ts#L433) but I should be able to make it async I think.

The tricky part is that on shutdown a user might decide to "Save All" from the confirm dialog and shutdown phase is a timing sensitive area (e.g. when you are also restarting the OS). So we would also need to introduce a threshold before we ignore the participants and just write to disk to not potentially have the OS kill VS Code and the user loosing data. I think a timeout would make sense in general anyway because we never know how long an extension might need to do the participant.

Today the only use case for the save participant is the "Trim Trailing Whitespace" command (https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/files/common/editors/saveParticipant.ts#L50). That participant is currently quite stupid (e.g. there is no registry to contribute more). I am fine to push either all extension and timeout logic into that guy or starting a new static registry (though that might be overkill).

[bpasero]

Upon more thinking about this, I think we should not apply edits directly on the model but let the extensions return edit operations they want to apply. Imagine this:

• user has auto save enabled
• user types and suddenly auto save kicks in which triggers a long running save participant operation
• user continues to type
• the long running save participant operation returns edits for a model that is no longer valid because it was changed meanwhile

Here is what I suggest:

• user types and suddenly auto save kicks in with save participants
• we collect all edit operations from all extensions
• the workbench file model makes a decision if these edits can be applied or not: if the model changed meanwhile, the edits will be dropped because it means the model is dirty again and the next save will trigger another round of save participants
• if the model was not changed, the workbench file model applies these edits before saving

Btw I would also suggest to let the "trim trailing whitespace" be a real extension so that we have a good use case for exercising the API. That would simplify the current vs/workbench/parts/files/common/editors/saveParticipant by just letting it collect all edits from all extensions that want to participate.

[jrieken]

I think we should not apply edits directly on the model but

Yes - that why I have proposed the API as pushEdits. In the extension API you cannot modify a document unless you have an editor or a way to pass on TextEdits.

[bpasero]

@jrieken tricky part is how to give multiple extensions a consistent view of the model? because if we ask the extensions for edits in a sequence, a previous extension could change the model in a way that the current extension produces wrong edits.

It would be nice if we could clone a model once we start asking extensions for edits, apply the edits on the clone to be able to give extensions a consistent view of the model and then return a merged set of edits to apply.

[jrieken]

multiple extensions a consistent view of the model

I think you don't. When the first listener make edits, the second listeners will see the n+1 state of the document and so on. I don't see us merging text edits from different listeners as they are likely to overlap and conflict in many ways.

[mysticatea]

I'm sorry, I had worked for other things while had been waiting for some response to my previous comment.

I had sneaked a bit; I understood this is harder than the first look.

• EventEmitter works in synchronous, but most use cases of onWillSaveTextDocument event are asynchronous. This is a mismatch.
• If a sync event executes multiple async jobs, the most lightweight job will complete edits at first, and then the heaviest job will repeat to try editing to resolve conflicts. This is bad.

---

My idea is API which is similar to event.waitUntil of Service Worker.

interface TextDocumentWillSaveEvent {
  document: TextDocument;
  token: CancellationToken;
  waitUntil(promise: Thenable<any>): void;
}

onWillSaveTextDocument: Event<TextDocumentWillSaveEvent>

• If promises are registered, EventEmitter pauses the event emitting loop until the promises get done.
  VSCode saves the document after the event emitting loop. This prevents the repeat of the heaviest job.
• Maybe VSCode can manage the set of extension's info and the promises. As a result, VSCode can show "'foo' is spending time in the pre-save process. [Cancel][Skip][Wait]"-like messages.
  
  
  
  • [Cancel] cancels the save action.
  
  
  • [Skip] cancels the promises and does the save action.
  
  
  • [Wait] leaves the process until the next timeout.
  
  

[jrieken]

If promises are registered, EventEmitter pauses the event emitting loop until the promises get done.

Exactly - that's what I am trying to say all the time ;-) Also, I do like waitUntil name.

[kube]

I know that you're already working on this, but I wanna expose an issue I have, and I think I will still have after this feature addition.

I have a little extension that adds me a custom header at top of my code.
This header is just composed by multiple comment lines, but left padded by spaces, and it adds an empty line at the top of my file.

https://github.com/kube/vscode-kube-header/blob/master/src/extension.ts

It's pretty simple, but my issue is that code formatters (like Go-format or TypeScript formatter) removes spaces and/or empty lines.
So my extension replaces a broken header by a new one.

I added a hook on save (as you can see in link), to refresh the header.

But, I really need to have this to be executed AFTER the Go-extension formatter. (Or it would be useless)

Would it be possible to add an order or priority parameter, or dependencies, or something to permit to be sure that a hook is executed AFTER or BEFORE some others.

[kumarharsh]

@kube - but won't this custom header get mangled on the _next_ manual save anyway since goformat will run on every save?

[kube]

@kumarharsh That's exactly why I wanna be able to define onWillSaveTextDocument handlers order.

onSave: GoFormat > UpdateHeader
is what I want as it will fix what GoFormat just broke.

onSave: UpdateHeader > GoFormat
would be useless, as GoFormat will break what UpdateHeader just did.

I need a way to set priority or dependency of tasks, Idk what solution would be the best though.

[jrieken]

@kube For me it sound like you should talk to the Go Format folks. If they think it's the correct behaviour nothing stops them of using the same or a higher priority. I am not convinced that priorities are the right thing as implementors are naturally bias towards _their_ work.

[jrieken]

@SamVerschueren I believe this is interesting for your final-newline extension. Further ideas? Feedback?

[SamVerschueren]

When the first listener make edits, the second listeners will see the n+1 state of the document and so on.

This sums it up quite well I believe.

I like the idea of just returning TextEdit objects that are applied by VS Code internally to the document and not applying the edits yourself.

Also 👎 on order or priority. Like mentioned before, every extension author will manipulate the order for their own benefit and for their own use case.

What will the maximum time be that can be spend in the pre-save phase?

---

Just something that came in my mind and might be nice (but optional). VS Code could do some housekeeping regarding the time spend in the pre-save event of an extension. It could then sort them and invoke the fast-running pre-save actions first.

[jrieken]

What will the maximum time be that can be spend in the pre-save phase?

We have not made a final decision. My ideas is to wait a short time (~1sec) and then to continue with saving. Unsure if we should try to apply _late_ text edits - it would trigger another [dirty,pre-save,save]-cycle (hopefully not endlessly 🙈). There is also the idea of using a different strategy for save-on-shutdown because in that case we and the OS are less patient.

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

Yeah, that's a cool idea. I'll give it some though.

[SamVerschueren]

There is also the idea of using a different strategy for save-on-shutdown

Maybe just don't invoke pre save actions on shutdown? I think it's more important to save the work of the user then applying extension edits.

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.

[bpasero]

Our current thinking is to not run any save participant when VS Code is shutting down.

[kube]

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

[jrieken]

@kube It's uncommon to expose other listeners during event dispatch and I don't understand the use-case for that

[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! :beers: