From e9a8af86e57837d340019d7f841ba8fa133cfcb4 Mon Sep 17 00:00:00 2001 From: Mel Bourgeois Date: Wed, 24 May 2023 01:53:11 -0500 Subject: [PATCH 1/9] Improve types for get(), set(), and MapType --- src/types/YMap.js | 52 +++++++++++++++++++++++++++++----------- tests/undo-redo.tests.js | 3 ++- tests/y-map.tests.js | 12 ++++++---- 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/src/types/YMap.js b/src/types/YMap.js index e2dd7a49..b1417140 100644 --- a/src/types/YMap.js +++ b/src/types/YMap.js @@ -21,12 +21,25 @@ import * as iterator from 'lib0/iterator' /** * @template T + * @typedef {Extract} StringKey + */ + +/** + * This works around some weird JSDoc+TS circular reference issues: https://github.com/microsoft/TypeScript/issues/46369 + * @typedef {boolean|null|string|number|Uint8Array|JsonArray|JsonObject} Json + * @typedef {Json[]} JsonArray + * @typedef {{ [key: string]: Json }} JsonObject + * @typedef {Json|AbstractType} MapValue + */ + +/** + * @template {Record} T * @extends YEvent> * Event that describes the changes on a YMap. */ export class YMapEvent extends YEvent { /** - * @param {YMap} ymap The YArray that changed. + * @param {YMap} ymap The YMap that changed. * @param {Transaction} transaction * @param {Set} subs The keys that changed. */ @@ -37,7 +50,7 @@ export class YMapEvent extends YEvent { } /** - * @template MapType + * @template {Record} MapType * A shared Map implementation. * * @extends AbstractType> @@ -76,7 +89,8 @@ export class YMap extends AbstractType { _integrate (y, item) { super._integrate(y, item) ;/** @type {Map} */ (this._prelimContent).forEach((value, key) => { - this.set(key, value) + // TODO: fix + this.set(/** @type {any} */ (key), value) }) this._prelimContent = null } @@ -97,7 +111,8 @@ export class YMap extends AbstractType { */ const map = new YMap() this.forEach((value, key) => { - map.set(key, value instanceof AbstractType ? /** @type {typeof value} */ (value.clone()) : value) + // TODO: fix + map.set(key, /** @type {any} */ (value instanceof AbstractType ? /** @type {typeof value} */ (value.clone()) : value)) }) return map } @@ -170,12 +185,12 @@ export class YMap extends AbstractType { /** * Executes a provided function on once on every key-value pair. * - * @param {function(MapType,string,YMap):void} f A function to execute on every element of this YArray. + * @param {function(MapType[keyof MapType],StringKey,YMap):void} f A function to execute on every element of this YArray. */ forEach (f) { this._map.forEach((item, key) => { if (!item.deleted) { - f(item.content.getContent()[item.length - 1], key, this) + f(item.content.getContent()[item.length - 1], /** @type {StringKey} */ (key), this) } }) } @@ -192,6 +207,7 @@ export class YMap extends AbstractType { /** * Remove a specified element from this YMap. * + * TODO: type to only allow deletion of optional elements in MapType * @param {string} key The key of the element to remove. */ delete (key) { @@ -205,17 +221,18 @@ export class YMap extends AbstractType { } /** + * @template {StringKey} Key + * @template {MapType[Key]} Value * Adds or updates an element with a specified key and value. - * @template {MapType} VAL * - * @param {string} key The key of the element to add to this YMap - * @param {VAL} value The value of the element to add - * @return {VAL} + * @param {Key} key The key of the element to add to this YMap + * @param {Value} value The value of the element to add + * @return {Value} */ set (key, value) { if (this.doc !== null) { transact(this.doc, transaction => { - typeMapSet(transaction, this, key, /** @type {any} */ (value)) + typeMapSet(transaction, this, key, value) }) } else { /** @type {Map} */ (this._prelimContent).set(key, value) @@ -224,13 +241,14 @@ export class YMap extends AbstractType { } /** + * @template {StringKey} Key * Returns a specified element from this YMap. * - * @param {string} key - * @return {MapType|undefined} + * @param {Key} key + * @return {MapType[Key]|undefined} */ get (key) { - return /** @type {any} */ (typeMapGet(this, key)) + return /** @type {MapType[Key]|undefined} */ (typeMapGet(this, key)) } /** @@ -273,3 +291,9 @@ export class YMap extends AbstractType { * @function */ export const readYMap = _decoder => new YMap() + + +/** @type {YMap<{ foo: { hello: [3, 4, { hiThere: null }] }; }>} */ +const foo = new YMap(); + +foo.has("") diff --git a/tests/undo-redo.tests.js b/tests/undo-redo.tests.js index 1844af86..af7054d4 100644 --- a/tests/undo-redo.tests.js +++ b/tests/undo-redo.tests.js @@ -329,6 +329,7 @@ export const testUndoUntilChangePerformed = _tc => { const yMap = new Y.Map() yMap.set('hello', 'world') yArray.push([yMap]) + /** @type {Y.Map<{ key: string }>} */ const yMap2 = new Y.Map() yMap2.set('key', 'value') yArray.push([yMap2]) @@ -342,7 +343,7 @@ export const testUndoUntilChangePerformed = _tc => { Y.transact(doc2, () => yArray2.delete(0), doc2.clientID) undoManager2.undo() undoManager.undo() - t.compareStrings(yMap2.get('key'), 'value') + t.compareStrings(yMap2.get('key') || '', 'value') } /** diff --git a/tests/y-map.tests.js b/tests/y-map.tests.js index e12d55c9..33221b64 100644 --- a/tests/y-map.tests.js +++ b/tests/y-map.tests.js @@ -19,14 +19,16 @@ export const testMapHavingIterableAsConstructorParamTests = tc => { t.assert(m1.get('number') === 1) t.assert(m1.get('string') === 'hello') + /** @type {Y.Map<{ object: { x: number; }; boolean: boolean; }>} */ const m2 = new Y.Map([ ['object', { x: 1 }], ['boolean', true] ]) map0.set('m2', m2) - t.assert(m2.get('object').x === 1) + t.assert(m2.get('object')?.x === 1) t.assert(m2.get('boolean') === true) + /** @type {Y.Map} */ const m3 = new Y.Map([...m1, ...m2]) map0.set('m3', m3) t.assert(m3.get('number') === 1) @@ -281,7 +283,7 @@ export const testGetAndSetAndDeleteOfMapPropertyWithThreeConflicts = tc => { */ export const testObserveDeepProperties = tc => { const { testConnector, users, map1, map2, map3 } = init(tc, { users: 4 }) - const _map1 = map1.set('map', new Y.Map()) + const _map1 = map1.set('map', /** @type {Y.Map<{ deepmap: Y.Map }>} */ (new Y.Map())) let calls = 0 let dmapid map1.observeDeep(events => { @@ -306,10 +308,10 @@ export const testObserveDeepProperties = tc => { const dmap2 = _map2.get('deepmap') const dmap3 = _map3.get('deepmap') t.assert(calls > 0) - t.assert(compareIDs(dmap1._item.id, dmap2._item.id)) - t.assert(compareIDs(dmap1._item.id, dmap3._item.id)) + t.assert(compareIDs(dmap1?._item?.id || null, dmap2._item.id)) + t.assert(compareIDs(dmap1?._item?.id || null, dmap3._item.id)) // @ts-ignore we want the possibility of dmapid being undefined - t.assert(compareIDs(dmap1._item.id, dmapid)) + t.assert(compareIDs(dmap1?._item?.id || null, dmapid)) compare(users) } From ea3f171876256e785e2dbaf08f26a857d38f24b7 Mon Sep 17 00:00:00 2001 From: Mel Bourgeois Date: Wed, 24 May 2023 02:25:09 -0500 Subject: [PATCH 2/9] Fix types of entries argument & _prelimContent --- src/types/YMap.js | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/types/YMap.js b/src/types/YMap.js index b1417140..330763d5 100644 --- a/src/types/YMap.js +++ b/src/types/YMap.js @@ -22,6 +22,18 @@ import * as iterator from 'lib0/iterator' /** * @template T * @typedef {Extract} StringKey + * + * Like keyof, but guarantees the returned type is a subset of string. + * `keyof` will include number and Symbol even if the input type requires string keys. + */ + +/** + * @template T + * @typedef {readonly { + * [K in keyof T]: [K, T[K]]; + * }[StringKey][]} EntriesOf + * + * Converts an object schema into a readonly array containing valid key-value pairs. */ /** @@ -59,12 +71,12 @@ export class YMapEvent extends YEvent { export class YMap extends AbstractType { /** * - * @param {Iterable=} entries - an optional iterable to initialize the YMap + * @param {EntriesOf=} entries - an optional iterable to initialize the YMap */ constructor (entries) { super() /** - * @type {Map?} + * @type {Map, MapType[StringKey]>?} * @private */ this._prelimContent = null @@ -88,9 +100,8 @@ export class YMap extends AbstractType { */ _integrate (y, item) { super._integrate(y, item) - ;/** @type {Map} */ (this._prelimContent).forEach((value, key) => { - // TODO: fix - this.set(/** @type {any} */ (key), value) + this._prelimContent?.forEach((value, key) => { + this.set(key, value) }) this._prelimContent = null } @@ -111,8 +122,7 @@ export class YMap extends AbstractType { */ const map = new YMap() this.forEach((value, key) => { - // TODO: fix - map.set(key, /** @type {any} */ (value instanceof AbstractType ? /** @type {typeof value} */ (value.clone()) : value)) + map.set(key, value instanceof AbstractType ? /** @type {typeof value} */ (value.clone()) : value) }) return map } @@ -185,7 +195,7 @@ export class YMap extends AbstractType { /** * Executes a provided function on once on every key-value pair. * - * @param {function(MapType[keyof MapType],StringKey,YMap):void} f A function to execute on every element of this YArray. + * @param {function(MapType[StringKey],StringKey,YMap):void} f A function to execute on every element of this YArray. */ forEach (f) { this._map.forEach((item, key) => { @@ -207,7 +217,6 @@ export class YMap extends AbstractType { /** * Remove a specified element from this YMap. * - * TODO: type to only allow deletion of optional elements in MapType * @param {string} key The key of the element to remove. */ delete (key) { @@ -291,9 +300,3 @@ export class YMap extends AbstractType { * @function */ export const readYMap = _decoder => new YMap() - - -/** @type {YMap<{ foo: { hello: [3, 4, { hiThere: null }] }; }>} */ -const foo = new YMap(); - -foo.has("") From 6802f8dd58dd12527a4aa2f2efc558ff038eb8ed Mon Sep 17 00:00:00 2001 From: Mel Bourgeois Date: Fri, 26 May 2023 21:27:34 -0500 Subject: [PATCH 3/9] Fix entries validation --- src/types/YMap.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/YMap.js b/src/types/YMap.js index 330763d5..8dec01ee 100644 --- a/src/types/YMap.js +++ b/src/types/YMap.js @@ -30,7 +30,7 @@ import * as iterator from 'lib0/iterator' /** * @template T * @typedef {readonly { - * [K in keyof T]: [K, T[K]]; + * [K in StringKey]: [K, T[K]]; * }[StringKey][]} EntriesOf * * Converts an object schema into a readonly array containing valid key-value pairs. From 4f49c581aa4ab7de9cfd7febbc0d989fa9ccaf8c Mon Sep 17 00:00:00 2001 From: Mel Bourgeois Date: Sat, 24 Feb 2024 12:39:52 -0600 Subject: [PATCH 4/9] fix type errors in y-map.tests --- src/types/YMap.js | 8 ++++---- tests/y-map.tests.js | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/types/YMap.js b/src/types/YMap.js index 16b07f2a..6374dfd7 100644 --- a/src/types/YMap.js +++ b/src/types/YMap.js @@ -65,7 +65,7 @@ export class YMapEvent extends YEvent { * A shared Map implementation. * * @extends AbstractType> - * @implements {Iterable<[string, MapType]>} + * @implements {Iterable<[StringKey, MapType[StringKey]]>} */ export class YMap extends AbstractType { /** @@ -176,7 +176,7 @@ export class YMap extends AbstractType { /** * Returns the values for each element in the YMap Type. * - * @return {IterableIterator} + * @return {IterableIterator} */ values () { return iterator.iteratorMap(createMapIterator(this._map), /** @param {any} v */ v => v[1].content.getContent()[v[1].length - 1]) @@ -185,7 +185,7 @@ export class YMap extends AbstractType { /** * Returns an Iterator of [key, value] pairs * - * @return {IterableIterator<[string, MapType]>} + * @return {IterableIterator<[StringKey, MapType[StringKey]]>} */ entries () { return iterator.iteratorMap(createMapIterator(this._map), /** @param {any} v */ v => /** @type {any} */ ([v[0], v[1].content.getContent()[v[1].length - 1]])) @@ -207,7 +207,7 @@ export class YMap extends AbstractType { /** * Returns an Iterator of [key, value] pairs * - * @return {IterableIterator<[string, MapType]>} + * @return {IterableIterator<[StringKey, MapType[StringKey]]>} */ [Symbol.iterator] () { return this.entries() diff --git a/tests/y-map.tests.js b/tests/y-map.tests.js index 92571c52..16faa107 100644 --- a/tests/y-map.tests.js +++ b/tests/y-map.tests.js @@ -14,7 +14,7 @@ import * as prng from 'lib0/prng' export const testIterators = _tc => { const ydoc = new Y.Doc() /** - * @type {Y.Map} + * @type {Y.Map>} */ const ymap = ydoc.getMap() // we are only checking if the type assumptions are correct From b97920bcbb515b906536efc56afdacd09ed8a442 Mon Sep 17 00:00:00 2001 From: Mel Bourgeois Date: Sat, 24 Feb 2024 20:50:03 -0600 Subject: [PATCH 5/9] add Doc as possible YMap value --- src/types/YMap.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/YMap.js b/src/types/YMap.js index 6374dfd7..60790bd2 100644 --- a/src/types/YMap.js +++ b/src/types/YMap.js @@ -40,7 +40,7 @@ import * as iterator from 'lib0/iterator' * @typedef {boolean|null|string|number|Uint8Array|JsonArray|JsonObject} Json * @typedef {Json[]} JsonArray * @typedef {{ [key: string]: Json }} JsonObject - * @typedef {Json|AbstractType} MapValue + * @typedef {Json|AbstractType|Doc} MapValue */ /** From c9995bda176d071f900d82102c37c81e330da251 Mon Sep 17 00:00:00 2001 From: Mel Bourgeois Date: Sat, 24 Feb 2024 20:51:09 -0600 Subject: [PATCH 6/9] constrain getMap type --- src/utils/Doc.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/Doc.js b/src/utils/Doc.js index 0d8d471b..554932e1 100644 --- a/src/utils/Doc.js +++ b/src/utils/Doc.js @@ -251,14 +251,14 @@ export class Doc extends Observable { } /** - * @template T + * @template {Record} T * @param {string} [name] * @return {YMap} * * @public */ getMap (name = '') { - return /** @type {YMap} */ (this.get(name, YMap)) + return /** @type {YMap} */ /** @type {any} */ (this.get(name, YMap)) } /** From 8c1d7c72b81c75814a0c1e706f345c7d3bf9e8bc Mon Sep 17 00:00:00 2001 From: Mel Bourgeois Date: Sat, 24 Feb 2024 20:51:31 -0600 Subject: [PATCH 7/9] fix type errors in tests --- tests/doc.tests.js | 13 ++++++++----- tests/undo-redo.tests.js | 3 ++- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/doc.tests.js b/tests/doc.tests.js index bb94819d..77cac8e4 100644 --- a/tests/doc.tests.js +++ b/tests/doc.tests.js @@ -114,6 +114,7 @@ export const testSubdoc = _tc => { doc.on('subdocs', subdocs => { event = [Array.from(subdocs.added).map(x => x.guid), Array.from(subdocs.removed).map(x => x.guid), Array.from(subdocs.loaded).map(x => x.guid)] }) + /** @type {Y.Map<{ a: Y.Doc; b: Y.Doc, c: Y.Doc; }>} */ const subdocs = doc.getMap('mysubdocs') const docA = new Y.Doc({ guid: 'a' }) docA.load() @@ -121,18 +122,18 @@ export const testSubdoc = _tc => { t.compare(event, [['a'], [], ['a']]) event = null - subdocs.get('a').load() + subdocs.get('a')?.load() t.assert(event === null) event = null - subdocs.get('a').destroy() + subdocs.get('a')?.destroy() t.compare(event, [['a'], ['a'], []]) - subdocs.get('a').load() + subdocs.get('a')?.load() t.compare(event, [[], [], ['a']]) subdocs.set('b', new Y.Doc({ guid: 'a', shouldLoad: false })) t.compare(event, [['a'], [], []]) - subdocs.get('b').load() + subdocs.get('b')?.load() t.compare(event, [[], [], ['a']]) const docC = new Y.Doc({ guid: 'c' }) @@ -156,7 +157,9 @@ export const testSubdoc = _tc => { Y.applyUpdate(doc2, Y.encodeStateAsUpdate(doc)) t.compare(event, [['a', 'a', 'c'], [], []]) - doc2.getMap('mysubdocs').get('a').load() + /** @type {Y.Map>} */ + const mysubdocs = doc2.getMap('mysubdocs') + mysubdocs.get('a')?.load() t.compare(event, [[], [], ['a']]) t.compare(Array.from(doc2.getSubdocGuids()), ['a', 'c']) diff --git a/tests/undo-redo.tests.js b/tests/undo-redo.tests.js index 27636aff..29d8e1ff 100644 --- a/tests/undo-redo.tests.js +++ b/tests/undo-redo.tests.js @@ -448,6 +448,7 @@ export const testUndoNestedUndoIssue = _tc => { */ export const testConsecutiveRedoBug = _tc => { const doc = new Y.Doc() + /** @type {Y.Map>>} */ const yRoot = doc.getMap() const undoMgr = new Y.UndoManager(yRoot) @@ -481,7 +482,7 @@ export const testConsecutiveRedoBug = _tc => { t.compare(yRoot.get('a'), undefined) undoMgr.redo() // x=0, y=0 - yPoint = yRoot.get('a') + yPoint = yRoot.get('a') || new Y.Map() t.compare(yPoint.toJSON(), { x: 0, y: 0 }) undoMgr.redo() // x=100, y=100 From 5d0da802b8413e2f5e6cbbd148b85c24f8ed6fd7 Mon Sep 17 00:00:00 2001 From: Mel Bourgeois Date: Sat, 24 Feb 2024 21:37:09 -0600 Subject: [PATCH 8/9] type output of map.toJSON --- src/types/YMap.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/types/YMap.js b/src/types/YMap.js index 60790bd2..f457b8aa 100644 --- a/src/types/YMap.js +++ b/src/types/YMap.js @@ -139,11 +139,11 @@ export class YMap extends AbstractType { /** * Transforms this Shared Type to a JSON object. * - * @return {Object} + * @return {Partial} */ toJSON () { /** - * @type {Object} + * @type {any} */ const map = {} this._map.forEach((item, key) => { From f22dbb7036a4cff3140a55cc7580e91d6538abd8 Mon Sep 17 00:00:00 2001 From: Mel Bourgeois Date: Sat, 24 Feb 2024 21:41:51 -0600 Subject: [PATCH 9/9] add test file for types --- tests/test-types.ts | 92 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 tests/test-types.ts diff --git a/tests/test-types.ts b/tests/test-types.ts new file mode 100644 index 00000000..5b119ecc --- /dev/null +++ b/tests/test-types.ts @@ -0,0 +1,92 @@ +/** + * This file serves to validate the public TypeScript API for YJS. + * + * It is not included in `npm run lint` or any other automated type checking, but can be used + * by those working on YJS types to ensure that the public-facing type interface remains valid. + * + * Any lines which are supposed to demonstrate statements that _would_ generate type errors + * should be clearly marked with the type error that is expected to result, to provide a + * negative test case. + */ + +import * as Y from "../dist/src/index"; + +/* + * Typed maps + * + * - Key names are autocompleted in first parameter of `get` and `set`. + * - `MapType` value types are constrained to valid Y.Map contents. + */ +type MyType = { + foo: string; + bar: number | null; + baz?: boolean; +}; + +// Constructor argument keys & values are typechecked, and keys are autocompleted. +// Multiple items for each key and partial initialization are allowed. +const map = new Y.Map([ + ["foo", ""], + ["foo", "even better"], + // ERROR: Type '["baz", number]' is not assignable to type '["foo", string] | ["bar", number | null] | ["baz", boolean | undefined]'. + ["baz", 3], +]); + +// Entries are still allowed to be omitted, so get() still returns | undefined. +const defaultMap = new Y.Map(); + +// `json` has a type of `Partial` +const json = defaultMap.toJSON(); + +// string | undefined +const fooValue = map.get("foo"); +// literal "hi" (string) +const fooSet = map.set("foo", "hi"); +// number | null | undefined +const barValue = map.get("bar"); +// ERROR: Argument of type '"hi"' is not assignable to parameter of type 'number | null'. +const barSet = map.set("bar", "hi"); +// ERROR: Argument of type '"bomb"' is not assignable to parameter of type 'keyof MyType'. +const missingGet = map.get("bomb"); +// Escape hatch: get() +const migrateGet = map.get("extraneousKey"); + +// ERROR: Type '' does not satisfy the constraint 'Record'. +const invalidMap = new Y.Map<{ invalid: () => void }>(); +const invalidMap2 = new Y.Map<{ invalid: Blob }>(); +// Arbitrarily complex valid types are still allowed +type ComplexType = { + n: null; + b: boolean; + s: string; + i: number; + u: Uint8Array; + a: null | boolean | string | number | Uint8Array[]; +}; +const complexValidType = new Y.Map< + ComplexType & { nested: ComplexType & { deeper: ComplexType[] } } +>(); + +/* + * Default behavior + * + * Provides basic typechecking over the range of possible map values. + */ +const untyped = new Y.Map(); + +// MapValue | undefined +const boop = untyped.get("default"); +// Still validates value types: ERROR: Argument of type '() => string' is not assignable to parameter of type 'MapValue'. +const moop = untyped.set("anything", () => "whoops"); + +/* + * `any` maps (bypass typechecking) + */ +const anyMap = new Y.Map(); + +// any +const fooValueAny = anyMap.get("foo"); +// literal "hi" (string) +const fooSetAny = anyMap.set("foo", "hi"); +// Allowed because `any` unlocks cowboy mode +const barSetAny = anyMap.set("bar", () => "hi");