Skip to content

Follow up on Meteors ticket "Reduce EJSON.clone's when observing query changes" #351

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SimonSimCity
Copy link
Contributor

@SimonSimCity SimonSimCity commented Mar 5, 2020

Meteor recently merged a new performance improvement (meteor/meteor#10864). After talking to the author, it looks like we here will need to rethink and update our copy of the observe_multiplex.js file (https://github.com/cult-of-coders/redis-oplog/blob/c5fdd259aa7bc722d8ac6bd0956e53bdc240558c/lib/mongo/ObserveMultiplex.js) according to the changes done in the version of Meteor: https://github.com/meteor/meteor/pull/10864/files#diff-c975a7828521c93b5659a448ddd62cfa

For now I've just copied changes of meteor/meteor#10864

I'm still in the course of testing it though ...

Make sure to run it on Meteor 1.10 which currently is a release candidate.

@SimonSimCity
Copy link
Contributor Author

@theodorDiaconu Reading through the code and the changes here made me very curious about this snippet: df7ece3#diff-20ded112a1c5c61da0cb2325b5671e12

var callback = handle['_' + callbackName];
// clone arguments so that callbacks can mutate their arguments
// We silence out removed exceptions
if (callback === 'removed') {
try {
callback.apply(null, EJSON.clone(args));
} catch (e) {
// Supressing `removed non-existent exceptions`
if (!isRemovedNonExistent(e)) {
throw e;
}
}
}

So ... the callback is checked to be the string removed and then you run a callback.apply() on this - well - string ... Shouldn't the check be on callbackName? And wouldn't then we also get an error because isRemovedNonExistent() is not defined? Maybe we should just remove this section, since it will never execute and we didn't have any report about errors since this was changed in April 2018.

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.

1 participant