yjs / y-prosemirror

ProseMirror editor binding for Yjs
https://demos.yjs.dev/prosemirror/prosemirror.html
MIT License
328 stars 115 forks source link

Conversion doesn't check types #116

Open andy-zhou opened 2 years ago

andy-zhou commented 2 years ago

Please save me some time and use the following template. In 90% of all issues I can't reproduce the problem because I don't know what exactly you are doing, in which environment, or which y-* version is responsible. Just use the following template even if you think the problem is obvious.

Checklist

Describe the bug A clear and concise description of what the bug is.

When converting from a Prosemirror Node to a YXmlFragment, y-prosemirror doesn't check that attributes it sets are actually strings.

const type = new Y.XmlElement(node.type.name)
for (const key in node.attrs) {
  const val = node.attrs[key]
  if (val !== null && key !== 'ychange') {
    type.setAttribute(key, val)[Kevin Jahns, 2 years ago: • revert toms refactor](https://sourcegraph.com/github.com/yjs/y-prosemirror/-/commit/e39c91e45e113220b5d66311a24c9397b7af0a25)
  }
}

This is problematic because a Prosemirror Node can have a non-string attribute, such as a number. Using YXmlElement.getElement(...) can therefore return a non-string which claims to be a string.

To Reproduce Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Open console
  4. See error

Expected behavior A clear and concise description of what you expected to happen.

Screenshots If applicable, add screenshots to help explain your problem.

Environment Information

Additional context Add any other context about the problem here.

andy-zhou commented 2 years ago

Ping on this. I think there's a bunch of options to fix this, but not sure how you think about backwards compatibility.