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

Replace Request lib with Got #4708

Merged
merged 20 commits into from Jan 15, 2020
Merged

Replace Request lib with Got #4708

merged 20 commits into from Jan 15, 2020

Conversation

@christian-bromann
Copy link
Member

christian-bromann commented Oct 30, 2019

Proposed changes

In order to reduce memory usage and being able to run WebdriverIO in the browser we want to replace the heavy request lib with something more lightweight like Got.

I ultimately decided to use Got because of this comparison table:
https://github.com/sindresorhus/got#comparison

Given that various parts of WebdriverIO (e.g. repl) relies on Node.JS APIs we won't get around having to bundle it anyway.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Reviewers: @webdriverio/technical-committee

@christian-bromann christian-bromann requested a review from webdriverio/project-committers Oct 30, 2019
@christian-bromann christian-bromann added this to In progress in WebdriverIO Fiddle Platform via automation Oct 30, 2019
@mgrybyk

This comment has been minimized.

Copy link
Member

mgrybyk commented Oct 30, 2019

hell yeah

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 31, 2019

Codecov Report

Merging #4708 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4708      +/-   ##
==========================================
- Coverage   99.42%   99.42%   -0.01%     
==========================================
  Files         210      210              
  Lines        5561     5543      -18     
  Branches     1204     1195       -9     
==========================================
- Hits         5529     5511      -18     
  Misses         29       29              
  Partials        3        3
Impacted Files Coverage Δ
packages/wdio-browserstack-service/src/service.js 100% <100%> (ø) ⬆️
...es/wdio-crossbrowsertesting-service/src/service.js 100% <100%> (ø) ⬆️
packages/wdio-testingbot-service/src/service.js 98.64% <100%> (-0.02%) ⬇️
packages/wdio-shared-store-service/src/client.js 100% <100%> (ø) ⬆️
packages/wdio-sauce-service/src/service.js 100% <100%> (ø) ⬆️
packages/wdio-shared-store-service/src/launcher.js 100% <100%> (ø) ⬆️
packages/wdio-sumologic-reporter/src/index.js 100% <100%> (ø) ⬆️
packages/webdriver/src/request.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c6672c...7ff0216. Read the comment docs.

@christian-bromann christian-bromann self-assigned this Oct 31, 2019
@christian-bromann christian-bromann marked this pull request as ready for review Oct 31, 2019
@christian-bromann

This comment has been minimized.

Copy link
Member Author

christian-bromann commented Oct 31, 2019

@webdriverio/project-committers this is ready for review now. I would love to release this (once merged) as beta so we can test it before pushing it to all people.

@mgrybyk

This comment has been minimized.

Copy link
Member

mgrybyk commented Nov 1, 2019

@christian-bromann couple of questions

  • do you have some more TODO items here?
  • what have you tested already and what do you think we should focus on the most?
  • can you please fix unit tests?
@christian-bromann christian-bromann added this to the v6 milestone Nov 1, 2019
@baruchvlz baruchvlz force-pushed the cb-replace-request branch from 8bf68c0 to 677615b Nov 30, 2019
@baruchvlz

This comment has been minimized.

Copy link
Member

baruchvlz commented Nov 30, 2019

@christian-bromann As we plan to use this in the fiddle, maybe finishing this migration might be worth it in order to have a smaller bundle size for the fiddle.

@dylang

This comment has been minimized.

Copy link

dylang commented Dec 2, 2019

@christian-bromann Got v10 has been released. Now 100% TypeScript and improved API.

https://github.com/sindresorhus/got/releases

@christian-bromann christian-bromann force-pushed the cb-replace-request branch from 677615b to 60186a2 Jan 3, 2020
@christian-bromann

This comment has been minimized.

Copy link
Member Author

christian-bromann commented Jan 3, 2020

@webdriverio/project-committers this can now be finally reviewed.

@mgrybyk

This comment has been minimized.

Copy link
Member

mgrybyk commented Jan 13, 2020

There are a lot of conflicts. Does it make sense to review the PR now?

@christian-bromann christian-bromann force-pushed the cb-replace-request branch from c3edc98 to c3102a3 Jan 14, 2020
@christian-bromann

This comment has been minimized.

Copy link
Member Author

christian-bromann commented Jan 14, 2020

@mgrybyk resolved all conflicts. Please have a look now.

WebdriverIO Fiddle Platform automation moved this from In progress to Review in progress Jan 15, 2020
@mgrybyk

This comment has been minimized.

Copy link
Member

mgrybyk commented Jan 15, 2020

Looks good and works for me in my project. Great job!

@christian-bromann christian-bromann force-pushed the cb-replace-request branch from c3102a3 to 7ff0216 Jan 15, 2020
@christian-bromann christian-bromann dismissed mgrybyk’s stale review Jan 15, 2020

Feedback was addressed and later approved

@christian-bromann christian-bromann merged commit 3feba29 into master Jan 15, 2020
6 checks passed
6 checks passed
License Compliance All checks passed.
Details
codecov/patch 100% of diff hit (target 99.42%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +0.57% compared to 5c6672c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
WebdriverIO Fiddle Platform automation moved this from Review in progress to Done Jan 15, 2020
@christian-bromann christian-bromann deleted the cb-replace-request branch Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.