Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Enable passing custom fetch function to BrowserHTTPRequest constructor #1422

Merged
merged 14 commits into from
Dec 6, 2018

Conversation

caisq
Copy link
Collaborator

@caisq caisq commented Dec 1, 2018

Towards fixing tensorflow/tfjs#410


This change is Reviewable

@caisq caisq requested a review from nsthorat December 3, 2018 14:34
@caisq
Copy link
Collaborator Author

caisq commented Dec 3, 2018

@nsthorat For context: this is the first of two PRs that will fix the issue in which tfjs-node users have to override global.fetch in order to load models via http/https.

Copy link
Contributor

@tafsiri tafsiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caisq a general question about this. Why go the subclassing approach instead of allowing a user to pass a fetch function to the load function?

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

Copy link
Collaborator Author

@caisq caisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tafsiri

The two approaches (subclassing vs. adding a new arg to the constructor of BrowserHTTPRequest should both help us achieve the same goal. But I feel the subclassing approach is a little cleaner because it avoids adding an additional arg toBrowserHTTPRequest constructor.
WDYT?

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

@nsthorat
Copy link
Contributor

nsthorat commented Dec 3, 2018

My 2c is that passing a function is more canonical JS than inheritance (and inheritance usually is more confusing anyways). I don't care that much though.

@caisq
Copy link
Collaborator Author

caisq commented Dec 5, 2018

@nsthorat @tafsiri I have updated the PR to add an additional argument to the constructor of BrowserHTTPRequest and the factory function browserHTTPRequest. This has the additional nice property that we need to export one fewer symbol.

Thanks for the suggestion. PTAL.

@caisq
Copy link
Collaborator Author

caisq commented Dec 6, 2018

Gentle ping @ reviewers. I may not have made it clear in the previous comment, but the inheritance pattern has been replaced with the additional arg.

Copy link
Contributor

@tafsiri tafsiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @caisq! LGTM

Reviewed 1 of 4 files at r1, 3 of 3 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @nsthorat)


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

      this.fetchFunc = fetch;
    } else {
      assert(typeof fetchFunc === 'function',  'Not a proper fetch function.');

Consider adding "Must pass a function that matches the signature of [link to fetch docs on MDN]


src/io/browser_http_test.ts, line 1036 at r2 (raw file):

  });

  it('Subclassing BrowserHTTPRequest and overriding fetchFunc', async () => {

Update test string since not doing the subclassing

@caisq caisq changed the title Enable subclassing BrowserHTTPRequest class to override fetch function Enable passing custom fetch function to BrowserHTTPRequest constructor Dec 6, 2018
Copy link
Collaborator Author

@caisq caisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @tafsiri and @nsthorat)


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

Previously, tafsiri (Yannick Assogba) wrote…

Consider adding "Must pass a function that matches the signature of [link to fetch docs on MDN]

Done.


src/io/browser_http_test.ts, line 1036 at r2 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Update test string since not doing the subclassing

Done.

@caisq caisq merged commit 2ff431b into tensorflow:master Dec 6, 2018
@caisq caisq deleted the node-fetch branch December 6, 2018 19:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants