Add tf.util.fetch.

[annxingyuan]

This PR fixes tensorflow/tfjs#1439

Changes

• Added tf.util.fetch
• Add node-fetch as dependency
• Remove code that looks for global fetch in BrowserHTTPRequest
• Spy on tf.util rather than global for IO tests

---

This change is 

[dsmilkov]

Reviewed 4 of 10 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @nsthorat)

---

package.json, line 65 at r1 (raw file):

    "test-integration": "./scripts/test-integration.js",
    "test-ci": "./scripts/test-ci.sh"
  },

missing a top-level browser field so other bundlers downstream know to ignore node-fetch (the bundlers of users who use tfjs-core as an npm package). See https://github.com/tensorflow/tfjs-data/blob/master/package.json#L62

---

src/util.ts, line 669 at r1 (raw file):

}

let systemFetch: Function;

move systemFetch right next to the fetch() function since that's where it is used as cache

---

src/util.ts, line 671 at r1 (raw file):

let systemFetch: Function;
const getSystemFetch = () => {
  let fetchFunc: Function;

no need for this temporary variable. "return result" directly at each if branch

---

src/util.ts, line 675 at r1 (raw file):

  if (ENV.global.fetch != null) {
    fetchFunc = ENV.global.fetch;
  } else {

combine the else and the if into an "else if"

---

src/util.ts, line 680 at r1 (raw file):

      fetchFunc = require('node-fetch');
    } else {
      throw new Error(`Unable to find fetch polyfill.`);

Unable to find the fetch() method. Please add your own fetch() function to the global namespace.

---

src/util.ts, line 696 at r1 (raw file):

 *
 * ```js
 * tf.util.fetch('path/to/resource')

since our code snippets are runnable, let's point to this something we know exists. E.g. const response = await fetch('https://unpkg.com/@tensorflow/tfjs'); (also use await in the code snippet for consistency with other code snippets)

---

src/util_test.ts, line 504 at r1 (raw file):

    const savedFetch = (global as any).fetch;
    // tslint:disable-next-line:no-any
    (global as any).fetch = () => {};

since you want this test to run in all environments, use tf.ENV.global instead of global

---

src/io/browser_http.ts, line 24 at r1 (raw file):

 */

import {assert, fetch as systemFetch} from '../util';

no need to rename as systemFetch?

[annxingyuan]

Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat)

---

package.json, line 65 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

missing a top-level browser field so other bundlers downstream know to ignore node-fetch (the bundlers of users who use tfjs-core as an npm package). See https://github.com/tensorflow/tfjs-data/blob/master/package.json#L62

Done

---

src/util.ts, line 669 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

move systemFetch right next to the fetch() function since that's where it is used as cache

Done

---

src/util.ts, line 671 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

no need for this temporary variable. "return result" directly at each if branch

Done

---

src/util.ts, line 675 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

combine the else and the if into an "else if"

Done

---

src/util.ts, line 680 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Unable to find the fetch() method. Please add your own fetch() function to the global namespace.

Done

---

src/util.ts, line 696 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

since our code snippets are runnable, let's point to this something we know exists. E.g. const response = await fetch('https://unpkg.com/@tensorflow/tfjs'); (also use await in the code snippet for consistency with other code snippets)

Done

---

src/util_test.ts, line 504 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

since you want this test to run in all environments, use tf.ENV.global instead of global

Done

---

src/io/browser_http.ts, line 24 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

no need to rename as systemFetch?

Done

[nsthorat]

Reviewed 5 of 10 files at r1, 4 of 4 files at r2.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @annxingyuan and @nsthorat)

---

package.json, line 25 at r2 (raw file):

    "@types/jasmine": "~2.5.53",
    "@types/node": "~9.6.0",
    "@types/node-fetch": "^2.1.2",

make this ~ so it matches the dep

---

src/util_test.ts, line 499 at r2 (raw file):

  });
});

add one more describeWithFlags that constrains to node and makes sure node-fetch fetch is called via a mock

---

src/util_test.ts, line 501 at r2 (raw file):

describe('util.fetch', () => {
  fit('should allow overriding global fetch', () => {

remove f in fit

---

src/io/browser_http.ts, line 225 at r2 (raw file):

   * the function will be bound to `window`, instead of `this`.
   */
  private getFetchFunc() {

let's remove this method, rename this.fetchFunc => this.fetch, and just call it directly where getFetchFunc is currently used

[dsmilkov]

Reviewed 2 of 10 files at r1, 1 of 4 files at r2.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @annxingyuan)

[annxingyuan]

@nsthorat Would you mind taking a look at the node-fetch test? Not sure whether it's what you had in mind. I'm not sure how to spy on node-fetch since it just exports a single function without a parent object - but the only way global.fetch gets defined as a function is if node-fetch is imported.

[nsthorat]

Reviewed 1 of 3 files at r3.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @annxingyuan)

---

src/util_test.ts, line 515 at r3 (raw file):

});

describeWithFlags('util.fetch node', NODE_ENVS, () => {

can you do this

import * as nodeFetch from ...
...
spyOn(nodeFetch, 'fetch').and.returnValue(...)l;
util.fetch('fake-url');
expect(nodeFetch.fetch).toHaveBeenCalled();

[annxingyuan]

@nsthorat I added the code for the test (link) but it fails because node-fetch just exports a function - there's no parent to spy on. Or am I getting the syntax wrong?