search

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

Fixes #32, update to xhr2 with integration tests #45

Closed naugtur closed 9 years ago

latentflip commented 9 years ago

Nice one! Thanks @naugtur. So just to be totally clear, the only change here is going to be that instead of the raw XHR object, resp will now be the wrapped one by xhr?

If so, I'm +1 on this, it's a cleaner api, we merge this and #24 at least, and make this a 2.0.0

Then we can look at what needs to happen in this modules dependents (model/rest-collection) to release those.

naugtur commented 9 years ago

To be precise: resp in this line: https://github.com/AmpersandJS/ampersand-sync/pull/45/files#diff-6c5c6e7b7946defd2d15d103e9a56d31L124 will now be a response object as described in https://github.com/Raynos/xhr#var-req--xhroptions-callback with a rawRequest field being a reference to the XMLHttpRequest object

No other changes visible from the user perspective.

latentflip commented 9 years ago

Cool, okay, before we merge this, I think we should look at checking it actually works under node, given that that was the purpose in the first place.

I've opened #46, where I'm switching out xhr for request in node, most of the tests pass but a couple fail. Still looking at why that might be.

latentflip commented 9 years ago

It looks like this doesn't happen in request:

so we just need to add that to ampersand-sync too, though, we could do that after merging this arguably, since adding node support won't be a breaking change

ref: #46

naugtur commented 9 years ago

I'm looking into running sync in node in regards to #21 and I have a prototype that I now need to run in node and see how it breaks ;)