web3-providers-http: Migrate from xhr2-cookies to cross-fetch · Pull Request #5085 · web3/web3.js

ghost · 2022-05-29T12:03:06Z

I have replaced the old xhr2-cookies library to cross-fetch since it was better, and the npm i for xhr2-cookies was broken for the latest node LTS version as well.

Working perfectly like before.

coveralls · 2022-05-29T12:17:18Z

Pull Request Test Coverage Report for Build 2495828271

0 of 0 changed or added relevant lines in 0 files are covered.
21 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.2%) to 72.001%

Files with Coverage Reduction	New Missed Lines	%
packages/web3-providers-http/lib/index.js	21	64.15%

Totals
Change from base Build 2460883276:	-0.2%
Covered Lines:	3383
Relevant Lines:	4424

💛 - Coveralls

ghost · 2022-05-29T12:37:51Z


+// Apply missing polyfill for IE
+require('es6-promise').polyfill();
+require('abortcontroller-polyfill/dist/polyfill-patch-fetch');


I have added the polyfills in case of compatibility, but I think CI makes some complain about it

ghost · 2022-05-29T12:38:21Z

    options = options || {};

-    this.withCredentials = options.withCredentials || false;
+    this.withCredentials = options.withCredentials;


This is no longer boolean https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#sending_a_request_with_credentials_included

ghost · 2022-05-29T12:39:15Z

 }

 export interface HttpProviderAgent {
-    baseUrl?: string;


Have removed it since this isn't supported by fetch as well as this is not a standard for Http Agent

ghost · 2022-05-29T12:39:59Z

+  return new Promise(function(resolve, reject) {
+    resolve(response);
+  });
+};


Simple version of fetch-mock function added

ghost · 2022-05-29T12:40:42Z


            provider.send({}, function (err, result) {
-                assert.equal(typeof result, 'object');
+                assert.equal(typeof result, 'undefined');


This should not be an object as we didn't had any url to fetch

nazarhussain · 2022-06-01T11:08:50Z

+            var agent = new https.Agent();
+            var options = {agent: {https: agent}};
+            var provider = new HttpProvider('http://localhost:8545', options);
+            var result = await provider._prepareRequest();


Should not _prepareRequest create only a function which unless invoked should not return a promise?

@nazarhussain You mean we should return the function like () => fetch(this.host, options) instead of the promise itself?

I believe the _prepareRequest should not be async. It should return the request object synchronously. And later on we should be using request.send() to actually send and await.

If that seems to be a big change, then we can rename the function to _prepareAndSendRequest that will make the context right.

ghost · 2022-06-01T11:47:40Z

@nazarhussain Resolved those three comments

nikoulai

Please check the failing tests (except for e2e_windows)

lgtm-com · 2022-06-03T18:41:41Z

This pull request introduces 1 alert when merging f0f1fa0 into 2c0324a - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

ghost · 2022-06-03T18:46:26Z

@jdevcs @avkos @luu-alex @spacesailor24 @nazarhussain @nikoulai I have fixed the testcase, would like to have a look at?

lgtm-com · 2022-06-03T22:42:03Z

This pull request introduces 1 alert when merging 4a9235f into 2c0324a - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

nazarhussain

@ayanamidev Thanks for your contributions. Left one small comment. Rest seems good to me.

There are two other points that need to address.

We don't allow community contributions to change the package-lock.json file. So please remove the change from that file in your PR. Some team member will merge the PR and include the changes in package-lock.json.
Add an entry to CHANGELOG.md describing what you are changing in this PR.

nazarhussain · 2022-06-13T12:41:40Z

+            var agent = new https.Agent();
+            var options = {agent: {https: agent}};
+            var provider = new HttpProvider('http://localhost:8545', options);
+            var result = await provider._prepareRequest();


I believe the _prepareRequest should not be async. It should return the request object synchronously. And later on we should be using request.send() to actually send and await.

If that seems to be a big change, then we can rename the function to _prepareAndSendRequest that will make the context right.

ghost · 2022-06-13T14:38:16Z

@nazarhussain @nikoulai @spacesailor24 Would like to ask your thought about removing _prepareRequest function for web3-providers-http. This function was xhr2-cookies specific and doesn't export it to the wild. So I think it makes sense to remove with the shift to cross-fetch since it will only return promise with the current code.

ghost · 2022-06-14T13:51:25Z

@nazarhussain @nikoulai @spacesailor24 Addressed comments

nazarhussain

@ayanamidev LGTM. Please add an entry into CHANGELOG.md describing the change.

ghost · 2022-06-14T14:10:04Z

@nazarhussain Oh, sorry fixed.

nazarhussain · 2022-06-22T18:17:50Z

@ayanamidev Please resolve the conflicts. Those probably will be in CHANGELOG.md. Then I would be able to merge the PR right away.

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export

…ers-http

ghost · 2022-06-23T02:57:28Z

@nazarhussain Done. Every conflicts resolved without changes from source code

* web3-providers-http: Migrate from xhr2-cookies to cross-fetch (#5085) * Update CHANGELOG.md * Migrate from xhr2-cookies to cross-fetch * Address comments for web3-providers-http * web3-providers-http: Prevent global leakage of this.connected * Return server side error without creating additional error object * Fixed type of http.Agent https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export * Removed unused variable * Remove unnecessary catch block * Remove unnecessary internal _prepareRequest function from web3-providers-http * ⬆️ Update package-lock.json * Update packages/web3-providers-http/src/index.js Co-authored-by: Junaid <86780488+jdevcs@users.noreply.github.com> * ⚰️ Remove dead code * 🎨 Improve the with credentials check Co-authored-by: Ayanami <ayanami0330@protonmail.com> Co-authored-by: Junaid <86780488+jdevcs@users.noreply.github.com>

ghost commented May 29, 2022

View reviewed changes

nazarhussain suggested changes Jun 1, 2022

View reviewed changes

nazarhussain added 1.x 1.0 related issues Review Needed Maintainer(s) need to review labels Jun 1, 2022

nazarhussain requested review from Spacesai1or, avkos, jdevcs, luu-alex and nikoulai June 1, 2022 11:09

ghost requested a review from nazarhussain June 1, 2022 15:11

nikoulai reviewed Jun 3, 2022

View reviewed changes

ghost requested a review from nikoulai June 3, 2022 18:45

avkos approved these changes Jun 6, 2022

View reviewed changes

nazarhussain suggested changes Jun 13, 2022

View reviewed changes

ghost requested a review from nazarhussain June 14, 2022 13:50

nazarhussain suggested changes Jun 14, 2022

View reviewed changes

ghost requested a review from nazarhussain June 14, 2022 14:10

nazarhussain approved these changes Jun 14, 2022

View reviewed changes

nazarhussain changed the base branch from 1.x to nh/cross-fetch-update June 22, 2022 18:15

nazarhussain added Needs Clarification Requires additional input and removed Review Needed Maintainer(s) need to review labels Jun 22, 2022

Ayanami added 9 commits June 23, 2022 11:56

Update CHANGELOG.md

803a625

Migrate from xhr2-cookies to cross-fetch

06cb173

Address comments for web3-providers-http

616d5af

web3-providers-http: Prevent global leakage of this.connected

04f4904

Return server side error without creating additional error object

bf9c3e9

Fixed type of http.Agent

3af7a91

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export

Removed unused variable

f0f556a

Remove unnecessary catch block

4bb11dd

Remove unnecessary internal _prepareRequest function from web3-provid…

26bc8d5

…ers-http

nikoulai approved these changes Jun 23, 2022

View reviewed changes

nazarhussain merged commit 258ea95 into web3:nh/cross-fetch-update Jun 28, 2022

nazarhussain mentioned this pull request Jun 28, 2022

web3-providers-http: Migrate from xhr2-cookies to cross-fetch #5179

Merged

14 tasks

This was referenced Jul 5, 2022

Backport HttpProvider changes from web3.js to fix handling server-side errors klaytn/caver-js#670

Merged

Added missing fetch api polyfill #5214

Closed

CCIP-Read + Wildcard resolution #4759

Closed

jdevcs mentioned this pull request Jul 15, 2022

Release 1.7.5 #5255

Merged

Jack-Works mentioned this pull request Aug 23, 2022

Crashes in Service Worker: XHR2 is not a constructor #5281

Closed

1 task

Uh oh!

Conversation

ghost commented May 29, 2022

Uh oh!

coveralls commented May 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 2495828271

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 1, 2022

Uh oh!

nikoulai left a comment

Choose a reason for hiding this comment

Uh oh!

lgtm-com Bot commented Jun 3, 2022

Uh oh!

ghost commented Jun 3, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com Bot commented Jun 3, 2022

Uh oh!

nazarhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 13, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 14, 2022

Uh oh!

nazarhussain left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 14, 2022

Uh oh!

nazarhussain commented Jun 22, 2022

Uh oh!

ghost commented Jun 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented May 29, 2022 •

edited

Loading

ghost commented Jun 3, 2022 •

edited by ghost

Loading

ghost commented Jun 13, 2022 •

edited by ghost

Loading