[core-http] avoid mutating global fetch #9880

jeremymeng · 2020-07-02T21:44:51Z

Use import node_fetch from "node-fetch" instead of import "node-fetch".

Resolves #5143.

Use `import node_fetch from "node-fetch" instead of `import "node-fetch"`.

jeremymeng · 2020-07-03T00:13:21Z

sdk/core/core-http/src/nodeFetchHttpClient.ts

@@ -4,21 +4,20 @@
 import * as tough from "tough-cookie";
 import * as http from "http";
 import * as https from "https";
-import "node-fetch";
+import node_fetch from "node-fetch";


used node_fetch to avoid conflict with the global fetch

xirzec · 2020-07-07T20:06:39Z

sdk/core/core-http/src/nodeFetchHttpClient.ts

 }

 const globalWithFetch = global as GlobalWithFetch;
 if (typeof globalWithFetch.fetch !== "function") {
-  const fetch = require("node-fetch").default;
-  globalWithFetch.fetch = fetch;
+  globalWithFetch.fetch = node_fetch;


but... why do this? Can't we just use node_fetch directly? This class is only used in node where fetch isn't defined.

I actually tried that first, however, the Request type defined in node-fetch isn't compatible with Request from DOM because it defined additional properties. node-fetch v3.0 doesn't seem to have these.

Can I safely suppress/ignore this error?

src/nodeFetchHttpClient.ts:91:23 - error TS2345: Argument of type 'RequestInfo' is not assignable to parameter of type 'import("/home/azuser/git/jssdk/common/temp/node_modules/.pnpm/@types/node-fetch@2.5.7/node_modules/@types/node-fetch/index").RequestInfo'. Type 'Request' is not assignable to type 'RequestInfo'. Type 'Request' is missing the following properties from type 'Request': context, compress, counter, follow, and 6 more. 91 return node_fetch(input, init);

I now realized that current PR still modifies global so won't fix the issue.

It seems like the trouble is export type CommonRequestInfo = Request | string; in fetchHttpClient yeah? I think we need to either define our own input interface or re-use node-fetch's

Our base class FetchHttpClient has this abstract method so we need to have two types that are compatible with both browsers and Node.

abstract async fetch(input: CommonRequestInfo, init?: RequestInit): Promise<Response>;

I could narrow down to common things that work for both DOM and NodeJs for input: change to input: string because it's the only type we ever use. However it seems harder to do similar for RequestInit as many things are incompatible.

url: string is more accurate

/cc @willmtemple who made changes around this previously.

The returned type Response is not compatible either between browser and Node. So we would need three types that are common/intersection of browser and Node.

So I got rid of the type errors by defining common types with some of the properties as any. Looks very hacky but not sure whether there are better ways.

jeremymeng · 2020-07-09T04:52:33Z

sdk/core/core-http/test/defaultHttpClientTests.node.ts

@@ -40,6 +40,7 @@ describe("defaultHttpClient (node)", function() {
    });

    const client = new DefaultHttpClient();
+    (client as any)._fetch = httpMock.getFetch();


I originally added a DefaultHttpClient constructor to allow passing in the mocked fetch, but felt changing public api just for testing seems too much.

Honestly I think I'd like the constructor parameter better, since at the moment it's very hard to find all the places that set the secret property, since TS can't alias through any and the language service will claim it's never set.

Changed to allow passing in fetch as a constructor parameter.

Changed again. Now mocking the fetch() instance method.

jeremymeng · 2020-07-13T16:44:41Z

@xirzec @bterlson would the current approch work, or there are better approaches?

jeremymeng · 2020-07-20T23:56:24Z

sdk/core/core-http/test/mockHttp.ts

+  // returns the locally mocked fetch instance
+  getFetch(): typeof node_fetch {
+    /// @ts-ignore
+    return this._fetch as typeof node_fetch;


I had to ignore a type error here otherwise, the fetch-mock type is not compatible with node-fetch.

xirzec

Sorry that this review fell off my radar. I think if we can address the commented issue this PR should be mergeable.

xirzec · 2020-07-30T18:58:41Z

sdk/core/core-http/review/core-http.api.md

@@ -167,10 +168,13 @@ export function createPipelineFromOptions(pipelineOptions: InternalPipelineOptio
 //
 // @public (undocumented)
 export class DefaultHttpClient extends FetchHttpClient {
+    constructor(_fetch?: typeof node_fetch);


this is the only part that feels bad to me. I don't think we should have test-only stuff in the default constructor, especially not fetch-specific details.

If you already have getFetch as a method of the class, can't you just use sinon to override that method during testing instead of passing the thing explicitly here?

Agree. I didn't like changing the constructor. I will explore using sinon.

getFetch() was private I think. However I found fetch() is simple enough so I mocked it instead.

thus get rid of the constructor parameter that is solely for testing purpose.

xirzec

Thanks for the update! This looks much nicer!

I feel like while we could still refactor this to be a little cleaner, it's not worth the effort since it's only test code that is messy now.

jeremymeng · 2020-07-31T20:47:30Z

I feel like while we could still refactor this to be a little cleaner, it's not worth the effort since it's only test code that is messy now.

Yeah, I have duplicated the getMockedHttpClient() and was thinking it might not be worth it to share just for two tests.

chradek

Had a minor comment you can take or leave. Looks good, ship it!

sdk/core/core-http/src/nodeFetchHttpClient.ts

Co-authored-by: chradek <51000525+chradek@users.noreply.github.com>

Porting fix from Azure/azure-sdk-for-js#9880. Use import node_fetch from "node-fetch" instead of import "node-fetch". Resolves Azure#383

[core-http] avoid mutating global fetch

412da98

Use `import node_fetch from "node-fetch" instead of `import "node-fetch"`.

jeremymeng requested review from bterlson, chradek, daviwil and xirzec as code owners July 2, 2020 21:44

jeremymeng commented Jul 3, 2020

View reviewed changes

xirzec reviewed Jul 7, 2020

View reviewed changes

jeremymeng force-pushed the no-global-fetch branch from 32b210d to 6d2b1fa Compare July 9, 2020 04:49

jeremymeng commented Jul 9, 2020

View reviewed changes

Use common types to fix incompatible types between browser and Node

4fbc638

jeremymeng force-pushed the no-global-fetch branch from 6d2b1fa to 4fbc638 Compare July 20, 2020 23:54

jeremymeng commented Jul 20, 2020

View reviewed changes

xirzec mentioned this pull request Jul 21, 2020

Electron app error: Failed to execute 'fetch' on 'Window': member signal is not of type AbortSignal. #5143

Closed

Update api review file

1686e1b

xirzec reviewed Jul 30, 2020

View reviewed changes

jeremymeng added 2 commits July 31, 2020 10:53

Use sinon to mock NodeFetchHttpClient.fetch

a69930f

thus get rid of the constructor parameter that is solely for testing purpose.

Merge remote-tracking branch 'upstream/master' into no-global-fetch

ee25f63

xirzec approved these changes Jul 31, 2020

View reviewed changes

chradek approved these changes Jul 31, 2020

View reviewed changes

sdk/core/core-http/src/nodeFetchHttpClient.ts Outdated Show resolved Hide resolved

Update sdk/core/core-http/src/nodeFetchHttpClient.ts

bb11738

Co-authored-by: chradek <51000525+chradek@users.noreply.github.com>

jeremymeng merged commit d66de69 into Azure:master Jul 31, 2020

jeremymeng deleted the no-global-fetch branch July 31, 2020 22:02

jeremymeng mentioned this pull request Aug 7, 2020

Bump core-http dependency version to v 1.1.6 #10487

Merged

xirzec mentioned this pull request Sep 22, 2020

Stop polluting the global namespace with fetch Azure/ms-rest-js#383

Closed

xirzec mentioned this pull request Nov 30, 2020

[core-http] enable no-prototype-builtins #12717

Merged

2 tasks

jeremymeng added a commit to jeremymeng/ms-rest-js that referenced this pull request Jan 29, 2021

Avoid mutating global fetch

763f66c

Porting fix from Azure/azure-sdk-for-js#9880. Use import node_fetch from "node-fetch" instead of import "node-fetch". Resolves Azure#383

jeremymeng mentioned this pull request Jan 29, 2021

Avoid mutating global fetch Azure/ms-rest-js#414

Merged

oliverlj mentioned this pull request May 3, 2022

global variable shadowed? JakeChampion/fetch#1026

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core-http] avoid mutating global fetch #9880

[core-http] avoid mutating global fetch #9880

jeremymeng commented Jul 2, 2020 •

edited

Loading

jeremymeng Jul 3, 2020

xirzec Jul 7, 2020

jeremymeng Jul 7, 2020

xirzec Jul 7, 2020

jeremymeng Jul 7, 2020

jeremymeng Jul 7, 2020

jeremymeng Jul 7, 2020

jeremymeng Jul 8, 2020

jeremymeng Jul 9, 2020

jeremymeng Jul 9, 2020

xirzec Jul 13, 2020

jeremymeng Jul 20, 2020

jeremymeng Jul 31, 2020

jeremymeng commented Jul 13, 2020

jeremymeng Jul 20, 2020

xirzec left a comment

xirzec Jul 30, 2020

jeremymeng Jul 30, 2020

jeremymeng Jul 31, 2020

xirzec left a comment

jeremymeng commented Jul 31, 2020

chradek left a comment

[core-http] avoid mutating global fetch #9880

[core-http] avoid mutating global fetch #9880

Conversation

jeremymeng commented Jul 2, 2020 • edited Loading

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

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

jeremymeng commented Jul 13, 2020

Choose a reason for hiding this comment

xirzec left a 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

xirzec left a comment

Choose a reason for hiding this comment

jeremymeng commented Jul 31, 2020

chradek left a comment

Choose a reason for hiding this comment

jeremymeng commented Jul 2, 2020 •

edited

Loading