Run xhr transformers after request interceptors and refactoring

aurelia / http-client

A simple, restful, message-based wrapper around XMLHttpRequest.

MIT License

62 stars 59 forks source link

Run xhr transformers after request interceptors and refactoring #74

Closed alexanderchr closed 9 years ago

alexanderchr commented 9 years ago

Transformers responsible for modifying the xhr are renamed xhrTransformers to distinguish them from transformers which modify the message (those defined in RequestBuilder and applied in HttpClient).
A base class RequestMessage is created to contain common functionality of HttpRequestMessage and JSONPRequestMessage
The xhr is opened and transformed at a later stage, after any request interceptors are run. This allows interceptors to modify the request and closes #60.

Sorry for mashing unrelated commits together, but as two of them are really minor i thought it would save some merging.

EisenbergEffect commented 9 years ago

@bryanrsmith Can you review this? Also, can you take this into account with respect to the fetch client as well? I know we can't make these apis identical, but it seems reasonable to try to carry concepts and behaviors across the two when possible.

bryanrsmith commented 9 years ago

Thanks, @alexanderchr! I think this has been bugging a lot of people. Would you mind making a couple cleanup changes?

We update the dist/ files at release time, and not with every commit. Could you reset those changes so the commits include only files in src/?
I agree that "xhrTransformers" is a clearer name, but that file also includes response transformers that are no longer appropriate under that name.
Would you mind updating the commit messages to conform to our commit message guidelines? Using that format makes it much easier for us to track changes for release notes and such.

Otherwise this looks good to me!

alexanderchr commented 9 years ago

Thank you very much for the feedback @bryanrsmith. I've fixed most of your comments and updated the commits.

I don't understand your second point though. As far as i can see there are no response transformers in src/transformers.js Am i missing something?

bryanrsmith commented 9 years ago

@alexanderchr Ah, my mistake. I was looking at contentTransformer, which doesn't mutate the xhr. I think it's close enough, though :-)

alexanderchr commented 9 years ago

That one bothered me as well. I reasoned that while not strictly an xhr transformer it is also something that should be executed just before the request. Maybe pre-request-transformers would be a better name?

bryanrsmith commented 9 years ago

@alexanderchr I think it's fine. I just misread the code. Even though it doesn't direct manipulate the XHR, I agree that it's perfectly clear and a reasonable name.

bryanrsmith commented 9 years ago

@EisenbergEffect Looks good to me! Please verify CLA and merge at your convenience.

EisenbergEffect commented 9 years ago

Verified. I will merge shortly and this will go out int he release, hopefully today.