Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bug where model ajaxConfig.beforeSend could be ignored #23

Merged
merged 1 commit into from
Nov 10, 2014
Merged

Fixed bug where model ajaxConfig.beforeSend could be ignored #23

merged 1 commit into from
Nov 10, 2014

Conversation

tomasztunik
Copy link
Contributor

Check for xhrFields on the ajaxConfig and override for beforeSend were overshadowing referencing beforeSend from the ajaxConfig on the params object and beforeSend was ignored if xhrFields was undefined.

@tomasztunik
Copy link
Contributor Author

Also a thought about beforeSend - in the xhrFields block you apply with this (sync) as context - wouldn't be it more valuable and useful to apply and bind to the model so that model data is accessible in the beforeSend? or alternatively accept model as second argument as currently I would have to bind or wrap beforeSend in model initialize method to be able to have access to the model instance from beforeSend method.

@tomasztunik
Copy link
Contributor Author

fixes #17

@kamilogorek
Copy link

lgtm +1

@wraithgar
Copy link
Contributor

Does this mean that xhrFields and beforeSend are mutually exclusive now?

@tomasztunik
Copy link
Contributor Author

with this fix beforeSend still gets called when xhrFields are provided -
check L87 in ampersand-sync.js - so they are not mutually exclusive.

On Mon Nov 10 2014 at 7:09:43 PM Michael Garvin notifications@github.com
wrote:

Does this mean that xhrFields and beforeSend are mutually exclusive now?


Reply to this email directly or view it on GitHub
#23 (comment)
.

@wraithgar
Copy link
Contributor

Perfect thank you @tomasztunik I missed that last read through.

+1 and thanks for including the test

@tomasztunik
Copy link
Contributor Author

no problem - ended up not needing it in the end but might save some trouble
somebody else in the future :)

On Mon Nov 10 2014 at 8:10:46 PM Michael Garvin notifications@github.com
wrote:

Perfect thank you @tomasztunik https://github.com/tomasztunik I missed
that last read through.

+1 and thanks for including the test


Reply to this email directly or view it on GitHub
#23 (comment)
.

wraithgar pushed a commit that referenced this pull request Nov 10, 2014
Fixed bug where model ajaxConfig.beforeSend could be ignored
@wraithgar wraithgar merged commit ab9add5 into AmpersandJS:master Nov 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants