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

Extern xhr error property #319

Merged
merged 1 commit into from
Apr 5, 2016

Conversation

wader
Copy link

@wader wader commented Apr 1, 2016

Make it possible to access the response when requests fails.

I tried to change to ['xhr'] = but then the tests failed because of type error

Hope it's correct to target v1.6.x branch.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@wader
Copy link
Author

wader commented Apr 1, 2016

Related to #317

@joeyparrish
Copy link
Member

Please read CONTRIBUTING.md and follow the instructions there.

@wader
Copy link
Author

wader commented Apr 4, 2016

I signed it!

On Fri, Apr 1, 2016 at 5:01 PM, googlebot notifications@github.com wrote:

Thanks for your pull request. It looks like this may be your first
contribution to a Google open source project. Before we can look at your
pull request, you'll need to sign a Contributor License Agreement (CLA).

Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify.
Thanks.


If you've already signed a CLA, it's possible we don't have your GitHub
username or you're using a different email address. Check your existing CLA
data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's
name.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@googlebot
Copy link

CLAs look good, thanks!

@wader wader force-pushed the extern-error-xhr-property branch 2 times, most recently from 017646a to 4a881b9 Compare April 4, 2016 12:42
@wader
Copy link
Author

wader commented Apr 4, 2016

@joeyparrish CLA signed and test added. But the test is kind of useless as currently unit tests only run on uncompiled source... i tried to run it on compiled source but then a lot of other unrelated tests fails 😞

@joeyparrish
Copy link
Member

Yeah, you're right. The new test doesn't really help anything in this case. We could go ahead and drop it, IMO.

@joeyparrish joeyparrish self-assigned this Apr 4, 2016
@joeyparrish joeyparrish added the type: bug Something isn't working correctly label Apr 4, 2016
Make it possible to access response when a request fail

shaka-project#317
@wader wader force-pushed the extern-error-xhr-property branch from 4a881b9 to 464384a Compare April 5, 2016 07:52
@wader
Copy link
Author

wader commented Apr 5, 2016

@joeyparrish Ok, spec file change is gone now

@joeyparrish joeyparrish merged commit e244a1a into shaka-project:v1.6.x Apr 5, 2016
@joeyparrish
Copy link
Member

Thanks!

@wader
Copy link
Author

wader commented Apr 6, 2016

@joeyparrish would it be possible to get a new 1.6.x package version to npm?

@wader
Copy link
Author

wader commented Apr 8, 2016

Im looking at the 2.0 code to see how we could access the response. My guess is that in lib/net/http_plugin.js the xhr instance needs to be assign to the error somehow or?

@joeyparrish
Copy link
Member

@wader, sure I can push a new v1.6.x release with bug fixes.

As for 2.0, I'd rather take another approach. You need the body of the HTTP response, correct? What if instead of passing up XHR, we attached the response text to error.data instead?

@joeyparrish
Copy link
Member

v1.6.5 is up on npm now. Thanks!

@wader
Copy link
Author

wader commented Apr 9, 2016

@joeyparrish Great!
Both body and status code would be nice

shaka-bot pushed a commit that referenced this pull request Apr 12, 2016
Requested in #319

Change-Id: I2f52519d7a45769a0d2db3c038c9c04cf7fb59c1
@joeyparrish
Copy link
Member

@wader, v2 now includes the response body in the error object. Thanks!

@wader
Copy link
Author

wader commented Apr 13, 2016

@joeyparrish Thanks! and now i see that status code was already there, nice

@wader wader deleted the extern-error-xhr-property branch April 13, 2016 07:46
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants