nikgraf / react-yjs

React hooks for Yjs
MIT License
60 stars 1 forks source link

Feedback #1

Closed dmonad closed 2 months ago

dmonad commented 2 months ago

First of all, thanks for this great library! I think this might become an essential part of Yjs<>React development :)

Since you asked for feedback, I have some suggestions for you here.

https://github.com/nikgraf/react-yjs/blob/de59ed720093c5a71499d66494a9c4b8a1b26802/packages/react-yjs/src/deepEqual.ts#L3

You could use import { equalityDeep } from 'lib0/function' instead, as this already ships with Yjs.

https://github.com/nikgraf/react-yjs/blob/de59ed720093c5a71499d66494a9c4b8a1b26802/packages/react-yjs/src/useY.ts#L7

You could use a template and return the typed information of the Y-Types here. This is how I would model this as a JSDoc comment:

/**
 * @template {any} YType
 * @typedef {YType extends YArray<infer V> ? Array<Y2Json<V>> : (YType extends YMap<infer VM> ? { [key: string]: Y2Json<VM> } : (YType extends YXmlFragment ? string : YType))} Y2Json
 */

Then you can do Y2Json<Y.Array<Y.Map<number>>> which would resolve to { [key: string]: number; }[].

nikgraf commented 2 months ago

Great feedback! thanks @dmonad

Made a PR: https://github.com/nikgraf/react-yjs/pull/2/files

One question: Was it on purpose or did you forget Y.Text in your type example? I noticed leaving it out leads to return Y.Text instead of string

When I add it, it behaves as I expected: https://github.com/nikgraf/react-yjs/pull/2/files#diff-09f55e5a060e95c60747ac7aaceed143bacc82a5f27abccd33dbe6e1d3242185R11

Also what about Y.XmlElement? Seems to convert the resulting type to string anyway, but I wonder why and currently added it anyway.

dmonad commented 2 months ago

Right. I forgot about Y.Text. the rest was intentional. Y.XmlElement inherits from Y.XmlFragment and Y.XmlText inherits from Y.Text.

nikgraf commented 2 months ago

fantastic, thanks!

implemented it and shipped a 2.0.0