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

Provide an 'always' callback #33

Closed mhamann closed 9 years ago

mhamann commented 9 years ago

Provide a callback that can be used no matter what the outcome of the request is. This is useful for always running some code after the request finishes (e.g. hiding a "loading" indicator).

wraithgar commented 9 years ago

Sorry this seems to have somehow gotten overlooked. Would you be able to add a test for this?

mhamann commented 9 years ago

Yes, let me work something up.

cdaringe commented 9 years ago

should this be executed post- error/success handler? I feel like that is more common behavior. i never used the jQ ajax.always(), but always sounds analogous to finally, and I frequently use the promise rsvp finally, which is generally used to exec after the fact.

just asking. :smile:

mhamann commented 9 years ago

Good question. In my mind, always isn't necessarily the same as finally, because it's not ending a promise chain. It's more of "just another" event handler for an XHR--one that happens to run in every case.

We could change the code to look like this, though it makes things a bit more complex:

https://gist.github.com/mhamann/19b229f604d1bb23e287

pgilad commented 9 years ago

Yes always should be executed only after error or success.

mhamann commented 9 years ago

I've modified the code to fire the always callback after success or error. Tests are included as well. Let me know if anyone has additional comments or suggestions, otherwise let's merge!

mhamann commented 9 years ago

@wraithgar any comments?

pgilad commented 9 years ago

+1

mhamann commented 9 years ago

@lukekarrys @latentflip @HenrikJoreteg can you help get this merged?

pgilad commented 9 years ago

@naugtur Lets try to get this in before ver 4.0.0

naugtur commented 9 years ago

Merging it would be a pain after all changes that happened. It's not a breaking change at any point, so it wouldn't hurt to postpone it. I added it to v4 milestone to keep track of it.

mhamann commented 9 years ago

@naugtur Please let me know if I can assist with the merge.

Seems like if we merged this as soon as I finished writing the tests, this conversation wouldn't even be necessary. :-)

latentflip commented 9 years ago

@mhamann sorry for the delay. Have reopened as #58, with changes cherry picked to master, hopefully retaining your authorship. Let's carry on over there.