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

Fix missing statusText and status code #39

Merged
merged 2 commits into from
Jan 25, 2016

Conversation

dhritzkiv
Copy link
Member

Before, resp didn't have statusText and status as properties. Instead, those properties were accessible through either resp.rawRequest or through the value returned from calling model.fetch (both, I believe are the xhr instance)

Related to #31

(this might be worth waiting for #38 to be accepted, so I could merge that into this branch, to avoid conflicts)

Before, `resp` didn't have `statusText` and `status` as properties. Instead, those properties were accessible through either `resp.rawRequest` or through the value returned from calling `model.fetch` (both, I believe are the xhr instance)

Related to AmpersandJS#31
@kamilogorek
Copy link

Looks fine. I also verified #38 for this one. Cheers!

@wraithgar
Copy link
Contributor

Looks good. I wonder if this is something that changed when we moved to xhr2 and also why test wouldn't have caught this.

@dhritzkiv
Copy link
Member Author

I think this was definitely cause by the switch to xhr2, where the response object's contents were changed.

The tests didn't catch this because the tests use manually-triggered error/success callbacks with augmented arguments. In that way the tests aren't accurate. It might be a good idea to switch to using a real but lightweight http server for the tests.

Anyway, I've merged the code with the last PR

@wraithgar
Copy link
Contributor

K, the testing is probably a separate issue. :shipit: imo

wraithgar added a commit that referenced this pull request Jan 25, 2016
Fix missing statusText and status code
@wraithgar wraithgar merged commit 462ab75 into AmpersandJS:master Jan 25, 2016
@wraithgar
Copy link
Contributor

published as v5.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants