Rethink namespacing of tx options

[mattmccutchen]

The readme says:

Note: the options can also be passed as follows: Players.insert({name: "New player"}, {tx: {instant: true}});. This can be used to avoid potential namespace collisions with other packages that use the same options hash, such as aldeed:collection2. As soon as an options hash is passed as the value for tx (instead of true), this package won't consider any other options except those in the hash.

What are the relevant scenarios here? If we add support for the standard multi (#80) and upsert (#84) options, it seems natural to me to pass them at the top level even if a tx object is used. In particular, a call to collection.upsert(sel, mod, {tx: {...}}) will get automatically turned into collection.update(sel, mod, {upsert: true, tx: {...}}), and it would be confusing if that didn't do an upsert.

[JackAdams]

What would happen in that case is the transactions package would only look at fields in the tx value, ignoring the rest of the top level fields in the main object passed, then it would remove the tx field before passing the object with all the rest of the fields to collection2 (say) or the native mutator methods to do their thing. So (if the package supported it) {upsert: true} would still work.

[mattmccutchen]

That would make sense, but it doesn't seem to be what the code does. It sets opt = opt.tx here and doesn't save the original opt anywhere. If you don't mind breaking any apps that might be relying on the current behavior, we can switch to the intended behavior; it just means passing two arguments through the code, opt and txOpt (where either txOpt == opt or txOpt == opt.tx), and making sure we're looking at the proper one in each place. But are name collisions enough of a risk to justify keeping this feature at all if we're breaking the old behavior anyway and no one has complained about the fact that the old behavior didn't actually avoid name collisions?

[JackAdams]

You're quite right. The original options don't get passed on to the mutator methods. That's an omission on my part - consequence of not using TDD and trying to write code to accommodate cases that my own apps don't use.

In answer to: "But are name collisions enough of a risk to justify keeping this feature at all?"

You're probably right that it's unnecessary. I guess my original motivation was that, with the pot-pourri of fields that could be used by mongo (e.g. upsert), the package (e.g. description, instant, overridePermissionCheck, etc.), and a whole host from collection2, the namespace might get a little cluttered.

I'm doubtful that anyone is using that particular feature and I'm happy to remove it with a note about a breaking change in the CHANGELOG.md. (Since it doesn't work as intended anyway.)