AmpersandJS / ampersand-sync

Provides sync behavior for updating data from ampersand models and collections to the server.
http://ampersandjs.com
MIT License
20 stars 28 forks source link

Change in ajaxConfig behavior #3

Closed xMartin closed 10 years ago

xMartin commented 10 years ago

I ran into an issue where params is undefined in my ajaxConfig function introduced by 4c93179963759e927db114afe8f2fb3cd58d811d after simply running npm update in my ampersand project. Either this is a regression and should be fixed or the change should be in a 1.1.0 version.

HenrikJoreteg commented 10 years ago

Shoot, sorry @xMartin we probably should've just done a major version bump due to unknown differences like this one. Checking out your fixes now...

HenrikJoreteg commented 10 years ago

The general idea of being able to set an ajaxConfig on the model is for overwriting purposes, that should happen as a last thing in the code so any of those manual overrides don't get overwritten again.

The current implementation is a regression in that sense :-/

I'll take a stab at a fix.

latentflip commented 10 years ago

Going to attempt to fix this to be compatible with both this and the jquery version, and patch release it tomorrow. Sorry about the delay :/

HenrikJoreteg commented 10 years ago

so... part of the idea of ajaxConfig is was the ability to fully overwrite any option before it gets used to make the request as a means of allowing any override, even if detrimental or crazy (perhaps you're hitting a crazy API you can't change).

if ajaxConfig is an object, it should be merged into the request config, but if ajaxConfig is a function i wonder if it shouldn't just be called with the entire config so the developer can direct manipulate and return that full request config object.

the reason i'm raising this is if you're looking at major version iterations, we might do this as well. for example, in the current implementation i could never programmatically determine that requests of a certain type shouldn't have an otherwise presend auth header (it's late, hopefully that makes sense).

Point is, I think supplying ajaxConfig as a method should give you 100% control of the options that get used for the request.

latentflip commented 10 years ago

I think the conclusion is that the current implementation is what we want for a v2 of ampersand-sync (as it should always have been) (with the caveat of the above PR for a minor additional option). Henrik's comment as to modifiying the request can be achieved by adding a beforeSend function to ajaxConfig, which will receive the raw xhr object for all the modification you could want to do.

So... if we merge the above, and bump this to a v2.0.0, we can bump ampersand-model, ampersand-collection-rest-mixin, and amperand-rest-collection by major versions too. I'll then deprecate 1.0.2 and 1.0.3 of this module to ensure at the very least a warning is mentioned.

latentflip commented 10 years ago

Also added info in the ampersand-sync readme (#5) so we can link to it from the deprecation notices.

latentflip commented 10 years ago

Bumped ampersand-sync and it's dependents by major versions, and added deprecation notices for 1.0.2 and 1.0.3.

Sorry for the pain this caused, we'll definitely be more careful moving forward!