assume insert callback is last arg instead of second

[aldeed]

This tiny change fixes an issue I'm having with my package, collection2. C2 overrides Meteor.Collection in a similar way to collection-hooks, but it adds support for options as the second argument of insert. It removes the options argument before passing to the real insert. However, it seems that Meteor always loads collection-hooks after collection2, which means that the collection-hooks insert method is run first, and it is confused by args[1] not being a function.

This simple change uses the last argument instead of the second argument, which is more future-proof coding anyway.

aldeed/meteor-collection2#50

[mizzao]

This change is probably okay, but what if you just specify api.use('collection-hooks', {weak: true}); in the package.js for collection2 so that if collection-hooks is also present, it gets loaded first?

[aldeed]

Yeah, thought of that, but as far as I know the bug with using atmosphere packages as weak dependencies hasn't been fixed yet. Which means I'd also have to add collection-hooks as a dependency in smart.json, and it would be installed for everyone even if they don't actually use it. Not a critical issue, but kind of annoying.

But yes, I would go that route if weak dependencies worked properly.

[matb33]

Yeah this looks good. First though I'll have update/remove match this approach.

Also I don't think Collection2 should even have to have the words "collection-hooks" anywhere in its code... It doesn't depend on our package at all and really this pull request is a favor to us ;)

[aldeed]

@matb33 @mizzao, actually there is another issue I didn't catch.

return _super.call(self, args[0], function (err, obj) {

This does not pass through all args, so C2 doesn't get the options argument anyway for async. This could be fixed in a similar way, by wrapping the provided callback, assigning the wrapper to the last arg, and then using _super.apply with args. That again would not break anything in c-hooks, and I can update my PR if you want, but if you'd rather have me take the weak dependency approach, I can do that. Like you said, though, there is no actual dependency.

[aldeed]

Since it was simple, I updated the PR to use apply, and I changed update/remove to match. The tests all pass, but I didn't look real closely at the other things being done with the args in each advice function.

[mjgallag]

I'm still relying on this pull request even with the latest version of collection2 and therefore I'm unable to upgrade to the latest version of collection-hooks. Is there a reason why this pull request hasn't been merged? Perhaps an alternative solution I should be making use of that makes this pull request unnecessary? Just trying to understand what's going on here, appreciate the help.

[matb33]

The only reason is because I haven't had the time to look at it carefully. I want to devote the proper attention to this one. Apologies for taking so long... that being said, I will make an effort to look at this ASAP

[mjgallag]

No worries at all and no rush @matb33, doesn't seem to be anything in the new version of collection-hooks that I need access to currently, so I'll simply continue to use this fork for now. I just wanted to make sure I wasn't missing something. Thanks!

[mizzao]

Note that meteor/meteor#1358 and meteor/meteor#1989 are a possible fix for this if addressed.

[aldeed]

@mizzao, yeah kind of, but I'm not sure if using a weak dependency is the best way to solve. It's not actually a weak dependency, I just need c-hooks to load first or allow an options object as the second arg. As far as I'm concerned, merging just the insert.js changes from this PR would be fine. Those changes should have absolutely no effect on anything, except for fixing the C2 errors. The update.js changes are the more potentially dangerous changes, so I would say just skip those. If it's not broke, don't fix it.

[matb33]

@aldeed I ended up implementing your changes line by line instead of by pull request as I wanted to understand exactly what was going on as I was implementing. I think I only made one change in the update hook. Everything worked quite well, nice work and thank you!

[aldeed]

Nice, thanks @matb33.

[mjgallag]

:+1: