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

Make request available in Interceptor.response/responseError #33

Closed
timbell opened this issue Jan 15, 2016 · 9 comments
Closed

Make request available in Interceptor.response/responseError #33

timbell opened this issue Jan 15, 2016 · 9 comments

Comments

@timbell
Copy link
Contributor

timbell commented Jan 15, 2016

Could the Interceptor.response and (especially) responseError methods be extended to include the original request? This would allow us to implement 401 handling with retry as can be done with http-client using HttpResponseMessage.requestMessage.

@bryanrsmith
Copy link
Contributor

Yes, good idea!

@bryanrsmith
Copy link
Contributor

I'll try to work on this soon, but in the meantime it would be a good feature for someone interested in contributing! Here's what it would probably take:

  1. Modify applyInterceptors to accept an optional array of additional arguments to the interceptors. Need to change the interceptor invocation to something like successHandler && x => interceptor.successHandler(x, ...interceptorArgs).
  2. Modify processResponse's parameter list to accept the response and pass it along to applyInterceptors.
  3. Include result in the arguments passed to processResponse
  4. Add a test or two to cover the new functionality.

@timbell
Copy link
Contributor Author

timbell commented Jan 18, 2016

I can have a shot at this sometime this week if you like?

@timbell
Copy link
Contributor Author

timbell commented Jan 21, 2016

I have prepared a branch with the changes but I don't have the rights to publish to the repository (I have just submitted the a CLA).

@EisenbergEffect
Copy link
Contributor

@bryanrsmith CLA is confirmed.
@timbell You should be able to create a pull request from your branch on GitHub. Then we can review it and merge.

@timbell
Copy link
Contributor Author

timbell commented Jan 21, 2016

Struggling to push local changes =>

git push origin include-original-request-in-interceptor-response-callbacks
remote: Permission to aurelia/fetch-client.git denied to timbell.
fatal: unable to access 'https://github.com/aurelia/fetch-client.git/': The requested URL returned error: 403

@gheoan
Copy link
Contributor

gheoan commented Jan 21, 2016

@timbell You have to first fork this repo.
Then add the fork as a remote.

git remote add fork https://github.com/timbell/fetch-client.git

Then git push fork include-original-request-in-interceptor-response-callbacks. Now you should see the 'submit pull request' button on https://github.com/timbell/fetch-client

It is common practice to git clone the fork so you have it as origin remote and if you need the forked repo you can add it as upstream remote.

timbell added a commit to timbell/fetch-client that referenced this issue Jan 21, 2016
… callbacks

feat(interceptor): pass original request as a second parameter to
response/responseError callbacks

Pass original request to response/responseError callbacks to support,
for example, handling 401 unauthorized errors by resubmitting the
request with added credentials

Closes aurelia#33
@timbell
Copy link
Contributor Author

timbell commented Jan 21, 2016

@gheoan - thanks. I've managed to submit something now - hopefully not too wrong...

timbell added a commit to timbell/fetch-client that referenced this issue Jan 22, 2016
@timbell timbell reopened this Jan 22, 2016
@timbell
Copy link
Contributor Author

timbell commented Jan 22, 2016

@bryanrsmith - sorry, I presume my commit comment closed this?
I've updated the pull request

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

No branches or pull requests

4 participants