feat(api): Implement Extmark API

[rhysd]

Fix #170

This PR adds Extmark API binding to Buffer. I confirmed it worked on my local:

const cp = require('child_process');
const { attach } = require('./packages/neovim');

const proc = cp.spawn('nvim', ['-u', 'NONE', '-N', '--embed'], {});

(async function () {
  const nvim = await attach({ proc });

  const buf = await nvim.buffer;
  await buf.replace(['this is test'], 0);

  const ns = await nvim.createNamespace('test');
  console.log('namespace:', ns);

  const id = await buf.setExtmark(ns, 0, 4);
  console.log('Mark ID:', id);

  const mark = await buf.getExtmarkById(ns, id);
  console.log('Mark:', mark);

  const marks = await buf.getExtmarks(ns, [0, 0], [0, 8]);
  console.log('Marks:', marks);

  const deleted = await buf.deleteExtmark(ns, id);
  console.log('Deleted:', deleted);

  const mark2 = await buf.getExtmarkById(ns, id);
  console.log('Mark after delete:', mark2);

  const marks2 = await buf.getExtmarks(ns, [0, 0], [0, 8]);
  console.log('Marks after delete:', marks2);

  nvim.quit();
  proc.kill();
})().catch(console.error);

Output:

namespace: 1
Mark ID: 1
Mark: [ 0, 4 ]
Marks: [ [ 1, 0, 4 ] ]
Deleted: true
Mark after delete: []
Marks after delete: []

[justinmk]

Is this generated? We should avoid manually writing interfaces that just map to nvim --api-info.

[rhysd]

Is this generated?

No, it was hand-written. I agree that generating APIs from nvim_api_info. This time, I prioritized Extmark API since it was requested by the issue.

I think it is a bit challenging to generate APIs. For example,

• Some parameter type is just an Array. Its element type is unknown
• Object property is snake_case in Vim script but it is lowerCamel in TypeScript. I'm not sure it is ok to convert cases of all properties automatically

[justinmk]

• Object property is snake_case in Vim script but it is lowerCamel in TypeScript. I'm not sure it is ok to convert cases of all properties automatically

Why don't we stick to the API names exactly? TypeScript "conventions" are not that important IMO.

[rhysd]

Why don't we stick to the API names exactly?

Since using snake_case looks weird. Actually no browser APIs and no Node.js APIs are using snake_case. Yes, it is just a convention, but I don't think it is not important.

[rhysd]

OK, I'll make a new issue for generating sources. Can we merge this PR at first? Then let me try to make some mechanism of code generation. (Though I'm not sure when I can take time to do it)

[justinmk]

Some parameter type is just an Array. Its element type is unknown

This doesn't sound like a blocker. Just type the items as any.

I think we should strictly stick with auto-generation if we're going to provide anything beyond request('nvim_foo', ...).

[justinmk]

Can we merge this PR at first?

Will we be able to remove this later once auto-generation is in place? That might be a breaking change.

[rhysd]

Will we be able to remove this later once auto-generation is in place? That might be a breaking change.

I think yes. Because the auto generated sources will break many other existing APIs. So taking care about only the Extmark API doesn't make much sense.

[justinmk]

Because the auto generated sources will break many other existing APIs.

why? we already were generating stuff in the past: https://github.com/neovim/node-client/blob/master/packages/neovim/bin/generate-typescript-interfaces.js

[rhysd]

why? we already were generating stuff in the past:

If my understanding is correct, this script is no longer used. The script was used before large changes by @billyvg.

Actually, as far as I ran the script on my local machine now, it only outputs 24 lines:

$ node ./bin/generate-typescript-interfaces.js                                                                                                                                                                                                                                               [Husky.local][10/13 21:56]
interface AttachOptions {
  writer?: NodeJS.WritableStream,
  reader?: NodeJS.ReadableStream,
  proc?: NodeJS.ChildProcess,
  socket?: String,
}
export default function attach(options: AttachOptions): Neovim;

export interface Neovim extends NodeJS.EventEmitter {
  quit(): void;
  isApiReady(): Boolean;
  requestApi(): Promise<[integer, any]>;
  equals(rhs: Neovim): boolean;
}
export interface Buffer {
  equals(rhs: Buffer): boolean;
}
export interface Window {
  equals(rhs: Window): boolean;
}
export interface Tabpage {
  equals(rhs: Tabpage): boolean;
}

[rhysd]

For example, this type has information more than the output from nvim_api_info. So it must be hand-written.

node-client/packages/neovim/src/api/Neovim.ts

Lines 72 to 125 in 635b642

	export interface OpenWindowOptions {
	// If set, the window becomes a floating window. The window will be placed with row,col coordinates relative to valueue
	// "editor" the global editor grid
	// "win" a window. Use 'win' option below to specify window id,
	// or current window will be used by default.
	// "cursor" the cursor position in current window.
	relative?: 'editor' | 'win' | 'cursor';
	// the corner of the float that the row,col position defines
	anchor?: 'NW' | 'NE' | 'SW' | 'SE';
	// Whether window can be focused by wincmds and mouse events. Defaults to true. Even if set to false, the window can still be entered using |nvim_set_current_win()| API call.
	focusable?: boolean;
	// `row`: row position. Screen cell height are used as unit. Can be floating point.
	row?: number;
	// `col`: column position. Screen cell width is used as unit. Can be floating point.
	col?: number;
	// when using relative='win', window id of the window where the position is defined.
	win?: number;
	// GUI should display the window as an external top-level window. Currently accepts no other positioning options together with this.
	external?: boolean;
	// width of the window
	width: number;
	// height of the window
	height: number;
	// Places float relative to buffer text (only when relative="win"). Takes a tuple of zero-indexed [line, column].
	bufpos?: [number, number];
	// Stacking order. floats with higher `zindex` go on top on floats with lower indices. Must be larger than zero
	zindex?: number;
	// Configure the appearance of the window.
	style?: 'minimal';
	// Style of (optional) window border. This can either be a string or an array.
	border?:
	| 'none'
	| 'single'
	| 'double'
	| 'rounded'
	| 'solid'
	| 'shadow'
	| string[];
	// If true then no buffer-related autocommand events such as |BufEnter|, |BufLeave| or |BufWinEnter| may fire from calling this function.
	noautocmd?: boolean;
	}

[billyvg]

Yeah I don't think we can 100% rely on autogeneration, but it'd be good to do so in order to keep up with neovim.

@rhysd did your code comments on the PR come from neovim or were they written by you?

re: camelcase vs snake, imo we should follow node/ts conventions which is generally camelCase.

[billyvg]

Suggested change

	* @param {ExtmarkPosition} start Start of range: a 0-indexed (row, col) or valid extmark id
	* (whose position defines the bound).
	* @param {ExtmarkPosition} start Start of range: a 0-indexed (row, col) or valid extmark id (whose position defines the bound).

[billyvg]

Suggested change

	* @param {ExtmarkPosition} end End of range (inclusive): a 0-indexed (row, col) or valid
	* extmark id (whose position defines the bound).
	* @param {ExtmarkPosition} end End of range (inclusive): a 0-indexed (row, col) or valid extmark id (whose position defines the bound).

[billyvg]

Suggested change

	* @param {BufferGetExtmarksOptions} options Optional parameters. Keys:
	* • limit: Maximum number of marks to return
	* • details: Whether to include the details dict
	* @param {BufferGetExtmarksOptions} options Optional parameters. Keys:
	* • limit: Maximum number of marks to return
	* • details: Whether to include the details dict

[billyvg]

Suggested change

	* • details: Whether to include the details dict
	* • details: Whether to include the details dict

[justinmk]

It's still not clear why this is needed. #170 isn't blocked because nodejs clients can do request('nvim_...", ...).

So why rush a hand-crafted API which we won't be able to revert later (breaking change)? Why not work on generating an API instead? What is blocking that? We generate all kinds of APIs in Nvim, is there a technical issue with doing that for nodejs client?

[justinmk]

This repo has lacked maintainer activity for 6+ months, which supports the point I made above about wanting to aggressively minimize the maintenance needed for this project.

node-client is important (used by projects like vscode-neovim). I want to keep node-client stable and useful, but I also want to aggressively avoid unnecessary potential for bugs and feature requests.