Better typing for outputs

[krassowski]

Problem

Currently outputs are not typed. This makes it hard to track changes in public API or to work with this package in general.

Previously at least the changes were typed as nbformat.IOutput but this changed in #241 and now they are just Map<any>:

jupyter_ydoc/javascript/src/api.ts

Lines 746 to 748 in 546a994

	* Cell output changes
	*/
	outputsChange?: Delta<Y.Map<any>>;

On the Python side the schema defines them as just an array, without a type:

jupyter_ydoc/jupyter_ydoc/ynotebook.py

Lines 35 to 46 in 546a994

	"cells": YArray[
	YMap[
	"id": str,
	"cell_type": str,
	"source": YText,
	"metadata": YMap,
	"execution_state": str,
	"execution_count": Int | None,
	"outputs": [] | None,
	"attachments": {} | None
	]
	]

[krassowski]

As per jupyterlab/jupyterlab#16764 (comment) this should be union of specialised Y.Map mostly following nbformat Output type but for stream type having Y.Array<text> specified.

[davidbrochart]

With a Y.Map<T> we only specify the type T of the values, because the type of the keys can only be string, right? If so, I don't understand how we can type an output 🤔

[krassowski]

Something along these lines?

interface YMap<MapType> extends Iterable<[string, MapType]> {
   toJSON(): Record<string, MapType>
   get(key: string): MapType | undefined;
}
const dummy: YMap<number> = {} as any;
dummy.get('this key may not exists')

interface CellFormat {
  source: string;
  index: number;
}

type CellKeys = keyof CellFormat;
type CellValues = CellFormat[CellKeys];

interface TypedMapExplicit extends YMap<CellValues> {
  get<K extends CellKeys>(key: K): CellFormat[K]
}
const clever: TypedMapExplicit = {} as any;
clever.get('source')  + 'a'
2 * clever.get('source')
clever.get('this key may not exists')

// Or if we want to be generic
interface TypedMapGeneric<Interface extends Record<string, any>> extends YMap<Interface[keyof Interface]> {
  get<K extends keyof Interface>(key: K): Interface[K]
}
const cleverGeneric: TypedMapGeneric<CellFormat> = {} as any;
cleverGeneric.get('source')
cleverGeneric.get('this key may not exists')

[krassowski]

It is a bit of work redefining the upstream methods to get them typed properly but on the upside TypeScript will catch any errors if we attempt to override them incorrectly. There are only two thing that can wrong:

• we forget to add types for a method (e.g. new method gets added to YMap upstream) - IMO not a big deal, it will still be better than status quo
• we make a typo in method name - IMO this will be caught by TS when we try to assign a YMap to the type so it should not be a problem; ideally we could use the override keyword but it is not yet supported on interfaces in TS: Add override / noImplicitOverride support for interfaces microsoft/TypeScript#45136 (if you agree it should be, please leave a 👍 on the issue)

[krassowski]

In parallel to fixing this here, maybe we should open an upstream issue to make an interface equivalent to TypedMapGeneric supported in y.js proper?

[davidbrochart]

I don't think I have enough knowledge of TypeScript to do this work at the moment, and it seems you have a pretty good idea of what has to be done 😄

[krassowski]

It seems state on YDocument might benefit from better typing too as per jupyterlab/jupyter-collaboration#411 (comment):

jupyter_ydoc/javascript/src/ydocument.ts

Lines 87 to 92 in c9525f1

	/**
	* Document state
	*/
	get state(): JSONObject {
	return JSONExt.deepCopy(this.ystate.toJSON());
	}