ampersand-sync messes up the options object

[naugtur]

options passed in to sync are being mutated in multiple ways, so it's not possible to keep one reference to a reusable options object and make all the sync calls passing this reference, because sync deletes from the object.

fix: copy the options object using optcopy = assign({},options)

What do you think about this idea?

[pgilad]

Yes, don't mutate the original options object... Create a new copy, either by assign or perhaps even clone or cloneDeep

[lukekarrys]

So it looks like at least ampersand-collection-rest-mixin relied on the options mutation to gain access to options.xhr. Here's the failing test on travis https://travis-ci.org/AmpersandJS/ampersand-collection-rest-mixin#L302

One fix I can think of without going back to mutating the options, is to assign the options back to request similar to what is done with ajaxSettings here: https://github.com/AmpersandJS/ampersand-sync/blob/master/core.js#L144

Then it would be up to the consuming modules to merge the options returned from sync with their original options if they wanted to. Thoughts on that?

[naugtur]

I didn't look into mixin code, but my thoughts are:

1. It's a major version bump afterall ;)
2. If tests depend on mutating options object (like sync tests did) then it has to be fixed
3. xhr is returned in request event and in callbacks and in synchronously from the sync function. Should cover all use-cases.

I'll look into it a bit more, but to me it sounds like a chance to eliminate a code smell from collection-rest-mixin

[lukekarrys]

@naugtur I was just looking at option 3 to try and grab the necessary stuff from request during the test, and you're right that should work. I'm working on that test now and it looks like we should be able to get it to pass without any sync changes. Yay for eliminating "required" implicit object mutation :smile: