Skip to content

Refactored the HTTP Client Implementation #483

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

Merged
merged 36 commits into from
Apr 19, 2019
Merged

Conversation

hiranya911
Copy link
Contributor

This code has become rather long and complex over the months. Therefore I'm refactoring it to make the code more modular and easier to follow:

  • Added an AsyncHttpCall class to handle a single HTTP interaction.
  • The old sendRequest() method has been broken down into a bunch of smaller methods, and moved into AsyncHttpCall.
  • Verified that all the existing unit tests pass, and added 2 new test cases to get 100% test coverage on the HTTP client code.

}
reject(enhanceError(err, config, null, req));
});
constructor(config: HttpRequestConfig) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to declare this as private since it is pretty much only initialized from within the static method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Done.

throw new Error('Request data must be a string, a Buffer or a json serializable object');
}

// Add Content-Length header if data exists
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@hiranya911
Copy link
Contributor Author

Thanks @bojeil-google. I've made the suggested changes.

@hiranya911 hiranya911 merged commit e7ee8a1 into master Apr 19, 2019
@hiranya911 hiranya911 deleted the hkj-http-cleanup branch April 19, 2019 18:07
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