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

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

packages/web3-providers-http/src/index.js

 var http = require('http');
 var https = require('https');

+// 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

packages/web3-providers-http/src/index.js


 /**
 * HttpProvider should be used to send rpc calls over http
 */
 var HttpProvider = function HttpProvider(host, options) {
    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

packages/web3-providers-http/types/index.d.ts

@@ -30,7 +30,6 @@ export interface HttpHeader {
 }

 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

test/httpprovider.js

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


Simple version of fetch-mock function added

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

test/httpprovider.js

    });

    describe('send', function () {
        it('should send basic async request', function (done) {
            var provider = new HttpProvider();

            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

packages/web3-providers-http/src/index.js

packages/web3-providers-http/types/index.d.ts

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

test/httpprovider.js

+            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

test/httpprovider.js

+            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 Review Needed labels Jun 1, 2022

nazarhussain requested review from jdevcs, avkos, luu-alex, nikoulai and spacesailor24 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 and removed Review Needed labels Jun 22, 2022

Ayanami added 9 commits June 23, 2022 11:56

Update CHANGELOG.md

803a625

Migrate from xhr2-cookies to cross-fetch

Verified

This commit was signed with the committer’s verified signature. The key has expired.

danielleadams Danielle Adams

GPG key ID: D3A89613643B6201
Expired

Verified
Learn about vigilant mode

06cb173

Address comments for web3-providers-http

Verified

This commit was signed with the committer’s verified signature. The key has expired.

danielleadams Danielle Adams

GPG key ID: D3A89613643B6201
Expired

Verified
Learn about vigilant mode

616d5af

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

Verified

This commit was signed with the committer’s verified signature.

ruyadorno Ruy Adorno

GPG key ID: E7DC4C5493C3DB5F

Verified
Learn about vigilant mode

04f4904

Return server side error without creating additional error object

Verified

This commit was signed with the committer’s verified signature.

ruyadorno Ruy Adorno

GPG key ID: E7DC4C5493C3DB5F

Verified
Learn about vigilant mode

bf9c3e9

Removed unused variable

Verified

This commit was signed with the committer’s verified signature.

ruyadorno Ruy Adorno

GPG key ID: E7DC4C5493C3DB5F

Verified
Learn about vigilant mode

f0f556a

Remove unnecessary catch block

Verified

This commit was signed with the committer’s verified signature.

ruyadorno Ruy Adorno

GPG key ID: E7DC4C5493C3DB5F

Verified
Learn about vigilant mode

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ghost commented May 29, 2022

coveralls commented May 29, 2022 •

edited

Loading

ghost May 29, 2022

ghost May 29, 2022

ghost May 29, 2022

ghost May 29, 2022

ghost May 29, 2022

nazarhussain Jun 1, 2022

ghost Jun 1, 2022

nazarhussain Jun 13, 2022

ghost commented Jun 1, 2022

nikoulai left a comment

lgtm-com bot commented Jun 3, 2022

ghost commented Jun 3, 2022 •

edited by ghost

Loading

lgtm-com bot commented Jun 3, 2022

nazarhussain left a comment

nazarhussain Jun 13, 2022

ghost commented Jun 13, 2022 •

edited by ghost

Loading

ghost commented Jun 14, 2022

nazarhussain left a comment

ghost commented Jun 14, 2022

nazarhussain commented Jun 22, 2022

ghost commented Jun 23, 2022

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

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

Conversation

ghost commented May 29, 2022

coveralls commented May 29, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2495828271

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 1, 2022

nikoulai left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 3, 2022

ghost commented Jun 3, 2022 • edited by ghost Loading

lgtm-com bot commented Jun 3, 2022

nazarhussain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 13, 2022 • edited by ghost Loading

ghost commented Jun 14, 2022

nazarhussain left a comment

Choose a reason for hiding this comment

ghost commented Jun 14, 2022

nazarhussain commented Jun 22, 2022

ghost commented Jun 23, 2022

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