Add a way to clean up (teardown) a plugin

[Milly]

Currently, there is no event to dispose the plugin on unload, such as before reloading plugin.
Allows the plugin to perform dispose processing.

The proposed API:

Returns teardown callback from main()

// Interface
export interface PluginModule {
    main: PluginMain | DisposablePluginMain;
}

export type PluginMain = (denops: Denops) => void | Promise<void>;
export type DisposablePluginMain = (denops: Denops) => TeardownLogic | Promise<TeardownLogic>;

export type TeardownLogic = () => void | Promise<void>;

// Plugin code
export const main: DisposablePluginMain = async (denops) => {
  const bufNr = await denops.call("bufadd", "my-buffer") as number;
  return async () => {
    await denops.cmd(`bwipeout ${bufNr}`);
  };
};

• Pros
  • This style is familiar to users of React hooks or RXJS.
• Cons
  • There will only be one TeardownLogic.
  • The TeardownLogic is anonymous, which makes it difficult to understand in the plugin code what it is.
  • The TeardownLogic will not run if main() throws an error.

Add subscriptions to Denops interface

// Interface
export type Subscriptions = {
  add(teardown: TeardownLogic): void;
  delete(teardown: TeardownLogic): void;
};

export type TeardownLogic = () => void | Promise<void>;

export interface Denops {
  /**
   * Adds or deletes the TeardownLogic that runs during plugin unload.
   *
   * TeardownLogic runs in the reverse order of registration.
   */
  subscriptions: Subscriptions
}

// Plugin code
export async function main(denops: Denops) {
  denops.subscriptions.add(async () => {
    if (bufNr != null) {
      await denops.cmd(`bwipeout ${bufNr}`);
    }
  });
  const bufNr = await denops.call("bufadd", "my-buffer") as number;
};

• Pros
  • Can add or delete any number of TeardownLogic at any time.
  • It can also support sub-Plugins within a Plugin. (e.g. ddu.vim)
• Cons
  • We need to update denops-core.

[Milly]

This issue may be partially replaced by #307's interrupt interface.

Dispose by AbortSignal

// Interface
export const DenopsInterruptReason = {
  PluginUnload: "PluginUnload",
} as const;

export interface Denops {
  interrupted: AbortSignal;
}

// Plugin code
export async function main(denops: Denops) {
  const attachTeardownLogic = () => {
    const { interrupted } = denops;
    interrupted.addEventListener("abort", async () => {
      if (interrupted.reason === DenopsInterruptReason.PluginUnload) {
        if (bufNr != null) {
          await denops.cmd(`bwipeout ${bufNr}`);
        }
      } else {
        // Re-attach because AbortSignal is re-assigned.
        setTimeout(attachTeardownLogic, 0);
      }
    });
  };
  attachTeardownLogic();
  const bufNr = await denops.call("bufadd", "my-buffer") as number;
};

• Pros

  • Implementation is simple. Only interrupt interface.
  • Can add or delete any number of TeardownLogic at any time.

• Cons

  • "abort" handlers are called in the order of registration. Not in reverse order. Affects resources release order.
  • Can specify an async callback as an "abort" handler, but there are problems.
    • Multiple registered handlers will not wait for another handler to finish before running.
    • The denops server will not wait for all callbacks to finish.
    • Exceptions raised within the callback are not handled.
  • Plugin code becomes complicated.

[lambdalisue]

Returns teardown callback from main()

I like it. It's simple and efficient for the purpose.

Add subscriptions to Denops interface

If we add subscriptions or similar, I'd like to make the things more generic so that we can add any kind of events for later.

Dispose by AbortSignal

I'm sorry but I don't think this is a good way because I think abort should be emitted only when the plugin is aborted. Not when plugin is successfully shutdown (for restart).

[Milly]

Returns teardown callback from main()

I like it. It's simple and efficient for the purpose.

I think so too.

• Multiple TeardownLogic processes can be implemented using separate libraries or logic.
• Errors within main() can be handled by the plugin author.

[lambdalisue]

📝 The word Disposable should not be used in this context while it is complicated with the Disposable (Symbol.dispose) in JavaScript.

[Milly]

I want to define the following interface as public. So that plugin developers can access it.

Please suggest adding this to denops-core or otherwise.

// Interface
export interface PluginModule {
    main: PluginMain;
}

export type PluginMain = (denops: Denops) => void | TeardownLogic | Promise<void | TeardownLogic>;

export type TeardownLogic = () => void | Promise<void>;

[Milly]

This may be a good definition for TeardownLogic that contains void as a pattern.

/**
 * Entry point function for denops plugin.
 *
 * @param denops: A facade instance of Denops visible from each denops plugin.
 * @returns A function called before the denops plugin is unloaded.
 */
export type PluginMain = (denops: Denops) => TeardownLogic | Promise<TeardownLogic>;

/**
 * The value that `{@link PluginMain}` should return.
 *
 * ## Function
 *
 * A function that takes no parameters. This function is called before the denops plugin is unloaded.
 *
 * ## void
 *
 * If the denops plugin has no resources to clean up, the function does not need to return anything.
 */
export type TeardownLogic = void | (() => void | Promise<void>);

[lambdalisue]

I want to define the following interface as public. So that plugin developers can access it.

Why? I think plugin developers don't need it.

[Milly]

I want to define the following interface as public. So that plugin developers can access it.

Why? I think plugin developers don't need it.

Of course, the type of TeardownLogic is simple, so developers can implement it just by reading the documentation. However, when I create a plugin, I want the type system to ensure that the return type of the main() function really extends with TeardownLogic.

[Milly]

Should we add events like DenopsSystemPluginTeardownPre, DenopsSystemPluginTeardownPost?

[lambdalisue]

Of course, the type of TeardownLogic is simple, so developers can implement it just by reading the documentation. However, when I create a plugin, I want the type system to ensure that the return type of the main() function really extends with TeardownLogic.

Are you suggesting a preference for the following approach?

// New approach
export const main: PluginMain = (denops) => {
  // ...
};

as opposed to

// Traditional approach
export function main(denops: Denops): void {
  // ...
}

Is this correct? Otherwise, exposing PluginMain or TeardownLogic seems unnecessary. It seems like a shift in recommendation, and such changes should be treated as a separate issue.

[Milly]

Are you suggesting a preference for the following approach?

// New approach
export const main: PluginMain = (denops) => {
  // ...
};

Yes, I want to write like this.
This is similar to React component definition.

[lambdalisue]

Got it. In that case, could you create a new issue for discussion? We'll handle exposing the types afterward. Since we'll need to work on deno-denops-core and deno-denops-std to expose the types, it's a bit of a side topic for this particular issue.

[lambdalisue]

Come to think of it, Disposable/AsyncDisposable could be used to write the following. I think this is better since it is closer to the standard, but what do you think?

type Promish<T> = T | Promise<T>;

export interface Module {
  main: Entrypoint;
}

export type Entrypoint = (
  denops: Denops
) => Promish<void | Disposable | AsyncDisposable>;

export const main: Entrypoint = (denops) => {
  // ...
  return {
    [Symbol.dispose]() {
      // ...
    },
  };
};

[Milly]

Come to think of it, Disposable/AsyncDisposable could be used to write the following. I think this is better since it is closer to the standard, but what do you think?

• Pros:
  • It is closer to the standard.
  • The plugin implementation is explicit because it references Symbol.dispose.
  • If already have an object that implements Disposable/AsyncDisposable, developer can simply returns it.
• Cons:
  • The plugin implementation is slightly more verbose than just returning an arrow function.

Overall, I agree.

[Milly]

Allow only AsyncDisposable instead Disposable/AsyncDisposable.
The reason is that it is not obvious whether both will be called when both exist, or which one will be called first in the case of both.

[Milly]

#385 is merged!