From 1aac245b93eb68e0ea9ac27b85d530b202dd6a91 Mon Sep 17 00:00:00 2001
From: Kevin Jahns <kevin.jahns@protonmail.com>
Date: Tue, 18 Jun 2019 17:41:19 +0200
Subject: [PATCH] New types dont fire events - fixes #155

---
 src/structs/ContentType.js |  1 -
 src/structs/Item.js        |  4 +++-
 src/utils/Transaction.js   | 46 ++++++++++++++++++++++++++++----------
 tests/y-array.tests.js     | 18 +++++++++++++++
 4 files changed, 55 insertions(+), 14 deletions(-)

diff --git a/src/structs/ContentType.js b/src/structs/ContentType.js
index fb0a69e4..dc9c3970 100644
--- a/src/structs/ContentType.js
+++ b/src/structs/ContentType.js
@@ -120,7 +120,6 @@ export class ContentType {
       }
     })
     transaction.changed.delete(this.type)
-    transaction.changedParentTypes.delete(this.type)
   }
   /**
    * @param {StructStore} store
diff --git a/src/structs/Item.js b/src/structs/Item.js
index 1675413b..7d821306 100644
--- a/src/structs/Item.js
+++ b/src/structs/Item.js
@@ -22,6 +22,7 @@ import {
   readContentEmbed,
   readContentFormat,
   readContentType,
+  addChangedTypeToTransaction,
   ContentType, ContentDeleted, StructStore, ID, AbstractType, Transaction // eslint-disable-line
 } from '../internals.js'
 
@@ -237,7 +238,8 @@ export class Item extends AbstractStruct {
     }
     addStruct(store, this)
     this.content.integrate(transaction, this)
-    maplib.setIfUndefined(transaction.changed, parent, set.create).add(parentSub)
+    // add parent to transaction.changed
+    addChangedTypeToTransaction(transaction, parent, parentSub)
     if ((parent._item !== null && parent._item.deleted) || (this.right !== null && parentSub !== null)) {
       // delete if parent is deleted or if this is not the current attribute value of parent
       this.delete(transaction)
diff --git a/src/utils/Transaction.js b/src/utils/Transaction.js
index 905f6723..d5ccc833 100644
--- a/src/utils/Transaction.js
+++ b/src/utils/Transaction.js
@@ -16,6 +16,7 @@ import {
 import * as encoding from 'lib0/encoding.js'
 import * as map from 'lib0/map.js'
 import * as math from 'lib0/math.js'
+import * as set from 'lib0/set.js'
 
 /**
  * A transaction is created for every change on the Yjs model. It is possible
@@ -117,6 +118,21 @@ export const nextID = transaction => {
   return createID(y.clientID, getState(y.store, y.clientID))
 }
 
+/**
+ * If `type.parent` was added in current transaction, `type` technically
+ * did not change, it was just added and we should not fire events for `type`.
+ *
+ * @param {Transaction} transaction
+ * @param {AbstractType<YEvent>} type
+ * @param {string|null} parentSub
+ */
+export const addChangedTypeToTransaction = (transaction, type, parentSub) => {
+  const item = type._item
+  if (item === null || (item.id.clock < (transaction.beforeState.get(item.id.client) || 0) && !item.deleted)) {
+    map.setIfUndefined(transaction.changed, type, set.create).add(parentSub)
+  }
+}
+
 /**
  * Implements the functionality of `y.transact(()=>{..})`
  *
@@ -153,20 +169,26 @@ export const transact = (doc, f, origin = null) => {
         doc.emit('beforeObserverCalls', [transaction, doc])
         // emit change events on changed types
         transaction.changed.forEach((subs, itemtype) => {
-          itemtype._callObserver(transaction, subs)
+          if (itemtype._item === null || !itemtype._item.deleted) {
+            itemtype._callObserver(transaction, subs)
+          }
         })
         transaction.changedParentTypes.forEach((events, type) => {
-          events = events
-            .filter(event =>
-              event.target._item === null || !event.target._item.deleted
-            )
-          events
-            .forEach(event => {
-              event.currentTarget = type
-            })
-          // we don't need to check for events.length
-          // because we know it has at least one element
-          callEventHandlerListeners(type._dEH, events, transaction)
+          // We need to think about the possibility that the user transforms the
+          // Y.Doc in the event.
+          if (type._item === null || !type._item.deleted) {
+            events = events
+              .filter(event =>
+                event.target._item === null || !event.target._item.deleted
+              )
+            events
+              .forEach(event => {
+                event.currentTarget = type
+              })
+            // We don't need to check for events.length
+            // because we know it has at least one element
+            callEventHandlerListeners(type._dEH, events, transaction)
+          }
         })
         doc.emit('afterTransaction', [transaction, doc])
         /**
diff --git a/tests/y-array.tests.js b/tests/y-array.tests.js
index 16321f0d..b3bb8454 100644
--- a/tests/y-array.tests.js
+++ b/tests/y-array.tests.js
@@ -210,6 +210,24 @@ export const testInsertAndDeleteEventsForTypes2 = tc => {
   compare(users)
 }
 
+/**
+ * This issue has been reported here https://github.com/y-js/yjs/issues/155
+ * @param {t.TestCase} tc
+ */
+export const testNewChildDoesNotEmitEventInTransaction = tc => {
+  const { array0, users } = init(tc, { users: 2 })
+  let fired = false
+  users[0].transact(() => {
+    const newMap = new Y.Map()
+    newMap.observe(() => {
+      fired = true
+    })
+    array0.insert(0, [newMap])
+    newMap.set('tst', 42)
+  })
+  t.assert(!fired, 'Event does not trigger')
+}
+
 /**
  * @param {t.TestCase} tc
  */