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

Default Accept header to JSON #24

Closed mikehedman closed 9 years ago

mikehedman commented 9 years ago

Sets the default ajax config header for Accept to 'application/json'.

mikehedman commented 9 years ago

This fixes https://github.com/AmpersandJS/ampersand-model/issues/18

dhritzkiv commented 9 years ago

:+1:

wraithgar commented 9 years ago

Can we throw up a quick test around this?

mikehedman commented 9 years ago

yeah, my bad. Let me work up a test or two.

bear commented 9 years ago

be aware @mikehedman that this PR will need to be updated because of some changes upstream

mikehedman commented 9 years ago

Yes, I'll deal with that now.

mikehedman commented 9 years ago

OK, I think it's good to go now.

wraithgar commented 9 years ago

+1 very good tests thank you!

lukekarrys commented 9 years ago

+1, thanks @mikehedman!

@wraithgar would you agree that this should be a minor release?

latentflip commented 9 years ago

+1 on this. Unsure about semver though. I would imagine this shouldn't break apps, but, could it?

mikehedman commented 9 years ago

In my opinion, it would be a very odd case, but yes there is the possibility that this could break an app. If someone had made an api call, saw that they get back XML, and then built around that fact, the switch to JSON would almost certainly cause their app to stop working. So from a literal semver point of view, it can break. In reality though, I think we would all assume that was pretty unlikely.

mikehedman commented 9 years ago

Sorry, hit the wrong button.

wraithgar commented 9 years ago

@mikehedman is right and this is a major release. As unlikely as this one is, 'breaking change' is a binary test not a probability.

latentflip commented 9 years ago

In that case let's add this to a milestone, and not merge it yet. Making breaking changes to sync is a pain in the butt, and I'm pretty sure we have some other bugs/issues we might like to fix in a major release.

It's a pain because releasing it means re-releasing almost all it's dependencies.

Philip Roberts &yet

On 12 Nov 2014, at 16:14, Michael Garvin notifications@github.com wrote:

@mikehedman is right and this is a major release. As unlikely as this one is, 'breaking change' is a binary test not a probability.

— Reply to this email directly or view it on GitHub.

latentflip commented 9 years ago

Where by dependencies I mean dependants.

Philip Roberts &yet

On 12 Nov 2014, at 16:14, Michael Garvin notifications@github.com wrote:

@mikehedman is right and this is a major release. As unlikely as this one is, 'breaking change' is a binary test not a probability.

— Reply to this email directly or view it on GitHub.

naugtur commented 9 years ago

Merged manually (actually, I copypasted code into right places, but the merge kept references to contributor)