Fix for conflicting cursor events

[gabrielcazacu96]

An attempt to fix conflicting cursor events in related to this grapher issue. Might break other things 🤷

This happens when the same document exists in the children of more than one parent cursor.

What happens:

1. added gets triggered from one parent
   1.1 docHash is updated with the new document
2. changed gets triggered from the other parent
   2.1 _shouldSendChanges is false because the same document already exists in docHash
   2.2 this.meteorSub.changed is not called

This is an issue because if this.meteorSub.changed is not called, the change is not sent through the DDP.

[StorytellerCZ]

Releasing in v1.8.7

[manueltimita]

@StorytellerCZ This needs to be reverted. The problem with this pull request is that const existingDoc = this.docHash[buildHashKey(collectionName, doc.id)] only seems to work because doc.id is undefined. Thus added will be executed unnecessarily, likely adding a large performance penalty.

@gabrielcazacu96 FYI, _hasDocChanged has exactly the logic you were looking for, no need to replace it - note the line const existingDoc = this.docHash[buildHashKey(collectionName, id)]:

  _hasDocChanged (collectionName, id, doc) {
    const existingDoc = this.docHash[buildHashKey(collectionName, id)]

    if (!existingDoc) { return true }

    return Object.keys(doc).some(key => !isEqual(doc[key], existingDoc[key]))
  }

[gabrielcazacu96]

@manueltimita

Sorry for the typo, you are right.

I still think _hasDocChanged should not be used there because it returns true if the object already exists in the hash but one of its fields has been updated. This is the reason i replaced it with const existingDoc = this.docHash[buildHashKey(collectionName, id)]

this._addDocHash(collectionName, doc) should only be triggered if the document does not exist at all, otherwise the changed event is not triggered.(This is an issue only to my case when the same document exists in the children of more than one parent cursor)

[manueltimita]

Thank you for clarifying, @gabrielcazacu96. I have two concerns, however:

1. Potential impact on republishing logic in _handleAddedAsync. Particularly, see the call to _republishChildrenOf(doc) for documents considered already published. Since added would no longer be called for updates to existing documents, we now rely on the correct invocation of changed to handle any updates. Do we know for sure that will work as expected?

2. The line you just changed has been there for over a decade, with lots of changes having happened around it, yet none touching it. I have a feeling there is a particular need for it to be like that, and added is just a misnomer. This is never a good enough reason to not touch a bug, but since this package has been through several maintainers, some knowledge has been lost along the way.

I have looked at the issue you mention at the top and I think the problem may very well have to do with mergebox, which only works on the top level fields only. Could it be that this is the reason changed is not triggered as you expect it? There are multiple examples of subscriptions not updating when an array is updated, like in this example.

It may look like a small change, but it has a systemic impact. I suggest moving this to a separate branch while testing in staging/production.

[UPDATE]
The more I think about it, the more I realise that likely the original author intended to copy the Meteor behaviour, for consistency. Let's put it this way, if I updated an array of objects and a simple Meteor publication did not push the changes, yet meteor-publish-composite did, I'd assume either one or the other has a bug.

[manueltimita]

@gabrielcazacu96 Please see #182. I have added several tests to check for the behaviour you are reporting, but so far I did not catch anything. I did my best to reproduce your original issue at cult-of-coders/grapher#481. If you think I missed something, feel free to add a test case that best describes the problem, and we'll take it from there.

[UPDATE]
Potentially related issue, see this answer: #153 (comment).

[gabrielcazacu96]

@manueltimita

I tried replicating the issue in the tests you've written but I'm having trouble replicating how the grapher package creates this kind of publication.

Also, the grapher issue seems to be fixed by @redabourial's #179. (Tested without the change in #177).

Thanks for taking the time to look into this. I think #177 can just be reverted since I was just fixing the symptoms but #179 seems to have fixed the central issue.