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

Remove request library #60

Merged
merged 7 commits into from
Mar 5, 2020
Merged

Remove request library #60

merged 7 commits into from
Mar 5, 2020

Conversation

ibalosh
Copy link
Contributor

@ibalosh ibalosh commented Mar 4, 2020

Hey @tomazy

could you take a quick look on this branch? Would love a pair of eyes to run through it even though its a small update, but touches main aspect of requests handling.

The update is related to http client library. RequestJS got deprecated. Due to popularity, ease of use, size, decided to go for axios.

Along the road error handling and tests were cleaned up a bit.

@ibalosh ibalosh requested a review from tomazy March 4, 2020 12:54
@ibalosh ibalosh self-assigned this Mar 4, 2020
@leeoniya
Copy link

leeoniya commented Mar 4, 2020

@ibalosh another option to evaluate is node-fetch, which is even smaller:

3.84MB: https://packagephobia.now.sh/result?p=request
419k: https://packagephobia.now.sh/result?p=axios
153k: https://packagephobia.now.sh/result?p=node-fetch

huge improvement, either way!

@leeoniya
Copy link

leeoniya commented Mar 4, 2020

i should add that i was considering moving our transactional emails to postmark until i looked at the lib size and compared it to using our own smtp servers with nodemailer:

4.09MB: https://packagephobia.now.sh/result?p=postmark
452k: https://packagephobia.now.sh/result?p=nodemailer

this change would definitely put postmark back in the running for me.

@ibalosh
Copy link
Contributor Author

ibalosh commented Mar 4, 2020

thats good to hear @leeoniya , update should be released in next couple of days

@lvnilesh lvnilesh mentioned this pull request Mar 4, 2020
Copy link

@tomazy tomazy left a comment

Choose a reason for hiding this comment

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

@ibalosh functionally it seems to work as before (however I haven't managed to run all the tests as green) so I think this is fine.

I usually try not to test private stuff so I"m not happy about those tests, but I see you mostly adjusted them to the new way of handling the requests so I guess we could think about improving this part on another occasion.

I've also left some stylistic suggestions but I leave it to you whether to apply them or not - they may interfere with the existing style of this project.

src/client/BaseClient.ts Outdated Show resolved Hide resolved
src/client/BaseClient.ts Outdated Show resolved Hide resolved
src/client/BaseClient.ts Outdated Show resolved Hide resolved
src/client/BaseClient.ts Outdated Show resolved Hide resolved
src/client/ErrorHandler.ts Show resolved Hide resolved
test/unit/AccountClient.test.ts Outdated Show resolved Hide resolved
src/client/ErrorHandler.ts Show resolved Hide resolved
test/unit/ServerClientErrors.test.ts Outdated Show resolved Hide resolved
test/unit/ServerClient.test.ts Outdated Show resolved Hide resolved
test/unit/ServerClient.test.ts Outdated Show resolved Hide resolved
@ibalosh
Copy link
Contributor Author

ibalosh commented Mar 5, 2020

Hey @tomazy , big thanks for code review. I implemented all the suggestions you made, they were all great!

@ibalosh ibalosh merged commit a2b014b into master Mar 5, 2020
@tomazy tomazy deleted the remove-request-library branch March 5, 2020 15:59
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