Skip to content

Conversation

fuzhuyi
Copy link

@fuzhuyi fuzhuyi commented Feb 1, 2021

Define yPrivate attribute in prosemirror schema, and prevent one client syncing the private attributes with other clients.

Copy link
Member

@dmonad dmonad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @fuzhuyi ,

The PR looks fine, but could you please make sure that npm test still works?

// remove all keys that are no longer in pAttrs
for (const key in yDomAttrs) {
if (pAttrs[key] === undefined) {
if (pAttrs.hasOwnProperties(key) && pAttrs[key] === undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition can't be true. So you are basically forcing that dom attributes are never deleted. This is probably why the tests fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this condition?

this._domSelectionInView = null

if (typeof getFallbackNode === 'function') {
const fallbackNode = getFallbackNode()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of code. Do you think it would suffice to simply store the name of the fallback node?

}

let node
if (typeof getFallbackNode === 'function') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified to a simple if/else construct.

// remove all keys that are no longer in pAttrs
for (const key in yDomAttrs) {
if (pAttrs[key] === undefined) {
if (key in pAttrs && pAttrs[key] === undefined) {
Copy link
Member

@dmonad dmonad Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment above. This condition can never be true.

const rightY = yChildren[yChildCnt - right - 1]
const rightP = pChildren[pChildCnt - right - 1]
if (leftY instanceof Y.XmlText && leftP instanceof Array) {
if (!(leftP instanceof Array) && isFallbackNode && isFallbackNode(leftP)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be simplified to simply: if (isFallbackNode(leftP)) {..}

if (!(leftP instanceof Array) && isFallbackNode && isFallbackNode(leftP)) {
if (mapping.get(leftY) !== leftP) {
yDomFragment.delete(left, 1)
yDomFragment.insert(left, /** @type Array{Y.XmlFragment} */ [leftY])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see why it is necessary to delete and insert the same node. Also, it is not possible to insert the same shared type (leftY) again in the document. Even if it has been deleted before.

} else {
Y.typeListToArraySnapshot(el, new Y.Snapshot(prevSnapshot.ds, snapshot.sv)).forEach(createChildren)
let isKnownNode = !!schema.nodes[el.nodeName]
if (isKnownNode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could simply do if (schema.nodes[el.nodeName]).

But you could simplify this even more by doing the following right at the beginning of createNodeFromYElement.

if (!schema.nodes[el.nodeName]) {
  let fallbackNode = schema.node(el.nodeName, attrs, children);
  mapping.set(el, fallbackNode);
  return fallbackNode
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants