Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed typo #180

Closed
Closed

Conversation

redabourial
Copy link
Contributor

doc.id here should be doc._id .

What

There is a bug causing all added documents to not be tracked correctly and outputing " (utils.js:14) Document ID is undefined" for each added document to stdout.

Why

The code seems to work correctly without fixing this issue (aside from the outputs to stdout). but since it is a typo it probably has some side effects i'm not aware of.

@manueltimita
Copy link
Contributor

manueltimita commented Mar 3, 2024

This has been introduced in the pull request here: #177. In my opinion, that kind of change should have been accompanied by a test, especially since it is meant specifically to address an issue with a third-party library (Grapher), not an inherent issue with meteor-publish-composite. Personally, I'm advocating for reverting that pull request until we have a better understanding of it.

[UPDATE]
@StorytellerCZ I know you asked me to review that pull requests, but I didn't have the time. I reviewed it now and it only seemingly worked for the implementer because of the bug highlighted here. The problem with #177 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.

For reference, here is the function whose call has been replaced by that call. Note that const existingDoc = this.docHash[buildHashKey(collectionName, id)] has been simply copied from there:

  _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]))
  }

[UPDATE 2]
Created pull request #181 to revert #177, likely rendering this pull request (#180) unnecessary.

[UPDATE 3]
There is an ongoing conversation under #177.

@redabourial
Copy link
Contributor Author

superseeded by #181 and the excellent work of @manueltimita

@redabourial redabourial closed this Mar 3, 2024
@manueltimita
Copy link
Contributor

Thanks @redabourial - check out #182

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