Fetch polyfill #17468

prateekbh · 2018-08-13T19:47:55Z

Writing a fetch polyfill to be used by AMP runtime.

Uses the existing fetch poly/pony fill
Adds a convertor function for standard Response creationg

…nto fet-polyfill

…et-polyfill

jridgewell · 2018-08-13T21:27:39Z

src/polyfills/fetch.js

+import {utf8Encode} from '../utils/bytes';
+
+/** @private @enum {number} Allowed fetch responses. */
+const allowedFetchTypes_ = {


Rename to allowedFetchTypes

jridgewell · 2018-08-13T21:27:56Z

src/polyfills/fetch.js

+import {parseJson} from '../json';
+import {utf8Encode} from '../utils/bytes';
+
+/** @private @enum {number} Allowed fetch responses. */


@private annotation has no meaning here.

jridgewell · 2018-08-13T21:30:05Z

src/polyfills/fetch.js

+ *   status: number,
+ *   statusText: string,
+ *   responseText: string,
+ *   responseXML: ?Document,


This is unneeded now.

jridgewell · 2018-08-13T21:32:07Z

src/polyfills/fetch.js

+ */
+class FetchResponse {
+  /**
+   * @param {!XMLHttpRequest|!XDomainRequest|!XMLHttpRequestDef} xhr


XDomainRequest is dropped.

jridgewell · 2018-08-13T21:51:17Z

src/polyfills/fetch.js

+    data.statusText = String(init.statusText);
+  }
+
+  return new FetchResponse(/** @type {XMLHttpRequestDef} */(data));


It's kinda weird that new Response() instanceof Response is false. I think the only difficulty to making Response be the main class is extracting out the headers from XML?

not really, i can make this a sub class of FetchResponse and just do super(blah)

Not really

Can you explain?

class Response extends FetchResponse{ constructor(body, init={}) { // all the current logic of functionResponse // instead of return new FetchResponse(/** @type {XMLHttpRequestDef} */(data)); super(/** @type {XMLHttpRequestDef} */(data)); } }

I understand the subclassing. I meant what else is the blocker for just using Response besides header parsing?

Ah ok, the fetchPolyfill is using FetchReponse as passing xhr in the constructor.
Just using Response class would just mean a re-write of that part and some additional code there

jridgewell · 2018-08-13T21:51:36Z

src/polyfills/fetch.js

+ */
+export function install(win) {
+  if (!win.fetch) {
+    win.prototype.fetch = /** @type {function(string, RequestInit):!Promise} */ (fetchPolyfill);


This should be on win, not win.prototype.

jridgewell · 2018-08-13T21:52:59Z

src/polyfills/fetch.js

+export function install(win) {
+  if (!win.fetch) {
+    win.prototype.fetch = /** @type {function(string, RequestInit):!Promise} */ (fetchPolyfill);
+    win.Response = Response;


Use Object.defineProperty

…nto fet-polyfill

jridgewell · 2018-08-14T19:19:04Z

src/polyfills/fetch.js

+ * serialized response returned by the viewer.
+ * @typedef {{
+ *   status: number,
+ *   statusText: string,


Doesn't look like this is used.

added to the FetchResponse

jridgewell · 2018-08-14T19:20:41Z

src/polyfills/fetch.js

+   * @param {!XMLHttpRequest|!XDomainRequest|!XMLHttpRequestDef} xhr
+   */
+  constructor(xhr) {
+    /** @private @const {!XMLHttpRequest|!XDomainRequest|!XMLHttpRequestDef} */


XDomainRequest again.

jridgewell · 2018-08-14T19:20:47Z

src/polyfills/fetch.js

+ */
+class FetchResponseHeaders {
+  /**
+   * @param {!XMLHttpRequest|!XDomainRequest|!XMLHttpRequestDef} xhr


XDomainRequest again.

woops, removed

jridgewell · 2018-08-14T19:24:06Z

src/polyfills/fetch.js

+     * @return {string}
+     */
+    getResponseHeader(name) {
+      return lowercasedHeaders[String(name).toLowerCase()] || null;


hasOwnProperty, since the value could be an empty string.

jridgewell · 2018-08-14T19:25:32Z

src/polyfills/fetch.js

+    data.statusText = String(init.statusText);
+  }
+
+  return new FetchResponse(/** @type {XMLHttpRequestDef} */(data));


Not really

Can you explain?

jridgewell · 2018-08-14T19:26:21Z

src/polyfills/fetch.js

+export function install(win) {
+  if (!win.fetch) {
+    win.fetch = /** @type {function(string, RequestInit):!Promise} */ (fetchPolyfill);
+    win.Response = Response;


Use Object.defineProperty.

is there a specific thing we want to configure with Object.defineProperty?

Object.getOwnPropertyDescriptor(window, 'Response') {value: ƒ, writable: true, enumerable: false, configurable: true} Object.getOwnPropertyDescriptor(window, 'fetch') {value: ƒ, writable: true, enumerable: true, configurable: true}

Enumerability, in particular, has been a source of countless polyfill bugs.

I am curious, can u list one such example?

https://developers.google.com/web/updates/2018/03/smooshgate, https://bugzilla.mozilla.org/show_bug.cgi?id=781796.

Gotcha! I have read that before, but now I got the complete understanding. Thanks

jridgewell · 2018-08-15T20:05:45Z

src/polyfills/fetch.js

+    /** @const {number} */
+    this.status = this.xhr_.status;
+
+    this.statusText = this.xhr_.statusText;


Needs annotations. But why add it?

It'll align better with the standard definition, without much effort.

jridgewell · 2018-08-15T20:06:46Z

src/polyfills/fetch.js

+       * @return {string}
+       */
+      getResponseHeader(name) {
+        return hasOwn(lowercasedHeaders, name) ?


name needs to be lowercased here, too.

jridgewell · 2018-08-15T20:08:52Z

src/polyfills/fetch.js

+  /**
+   * Returns instance of Response.
+   * @param {?ResponseBodyInit=} body
+   * @param {!ResponseInit=} init


Where are ResponseInit and ResponseBodyInit defined?

They are native to closure compiler.

jridgewell · 2018-08-15T20:10:18Z

src/polyfills/fetch.js

+      },
+    }, init);
+
+    init.status = init.status || 200;


Is this necessary? It also overrides a status of 0.

fixed it for only undefined. because closure's types say it can be a number or undefined

jridgewell · 2018-08-15T20:11:06Z

src/polyfills/fetch.js

+            String(init.headers[key]);
+      }
+    }
+    if (init.status) {


We already did this above?

jridgewell · 2018-08-15T20:11:45Z

src/polyfills/fetch.js

+    Object.defineProperty(win, 'fetch', {
+      value: fetch,
+      writable: true,
+      enumerable: false,


fetch is enumerable. Also configurable.

jridgewell · 2018-08-15T20:12:32Z

src/polyfills/fetch.js

+    Object.defineProperty(win, 'Response', {
+      value: Response,
+      writable: true,
+      enumerable: false,


configurable: true

prateekbh

Thanks the exhaustive review... 👍

prateekbh · 2018-08-15T21:13:26Z

src/polyfills/fetch.js

+    /** @const {number} */
+    this.status = this.xhr_.status;
+
+    this.statusText = this.xhr_.statusText;


It'll align better with the standard definition, without much effort.

prateekbh · 2018-08-15T21:13:41Z

src/polyfills/fetch.js

+  /**
+   * Returns instance of Response.
+   * @param {?ResponseBodyInit=} body
+   * @param {!ResponseInit=} init


They are native to closure compiler.

prateekbh · 2018-08-15T21:13:49Z

src/polyfills/fetch.js

+       * @return {string}
+       */
+      getResponseHeader(name) {
+        return hasOwn(lowercasedHeaders, name) ?


prateekbh · 2018-08-15T21:14:26Z

src/polyfills/fetch.js

+      },
+    }, init);
+
+    init.status = init.status || 200;


fixed it for only undefined. because closure's types say it can be a number or undefined

prateekbh · 2018-08-15T21:14:43Z

src/polyfills/fetch.js

+            String(init.headers[key]);
+      }
+    }
+    if (init.status) {


prateekbh · 2018-08-15T21:15:20Z

src/polyfills/fetch.js

+    Object.defineProperty(win, 'fetch', {
+      value: fetch,
+      writable: true,
+      enumerable: false,


prateekbh · 2018-08-15T21:15:25Z

src/polyfills/fetch.js

+    Object.defineProperty(win, 'Response', {
+      value: Response,
+      writable: true,
+      enumerable: false,


* reverting skip * adding fetch polyfill * WIP polyfill tests * WIP fetch-polyfill tests * passing tests * adding more test * ading more tests * ading more tests * Update dep-check-config.js * Update polyfills.js * resolving comments * removing extra test * response as a class * resolving comments * fixing type related issues * renaming file to avoid test conflict * adding copyright

prateekbh added 18 commits July 27, 2018 10:11

reverting skip

07ab01a

Merge branch 'master' of https://github.com/ampproject/amphtml

aeb82c7

Merge branch 'master' of https://github.com/ampproject/amphtml

e2c8a4c

Merge branch 'master' of https://github.com/ampproject/amphtml

b66c7dc

Merge branch 'master' of https://github.com/ampproject/amphtml

7a01fb1

Merge branch 'master' of https://github.com/ampproject/amphtml

44a22ea

Merge branch 'master' of https://github.com/ampproject/amphtml

b0f977b

Merge branch 'master' of https://github.com/ampproject/amphtml

eba72d4

Merge branch 'master' of https://github.com/ampproject/amphtml

8847257

adding fetch polyfill

9fb558d

WIP polyfill tests

d013c77

WIP fetch-polyfill tests

7867b5d

passing tests

7d87890

adding more test

be0fd42

ading more tests

73bb796

ading more tests

0396f76

Merge branch 'fet-polyfill' of https://github.com/prateekbh/amphtml i…

8ba28e3

…nto fet-polyfill

Merge branch 'master' of https://github.com/ampproject/amphtml into f…

28e5af3

…et-polyfill

googlebot added the cla: yes label Aug 13, 2018

prateekbh added 2 commits August 13, 2018 12:48

Update dep-check-config.js

c8a837a

Update polyfills.js

d4b30e0

prateekbh requested a review from jridgewell August 13, 2018 19:53

jridgewell reviewed Aug 13, 2018

View reviewed changes

prateekbh added 3 commits August 14, 2018 12:00

resolving comments

d6e98f5

Merge branch 'fet-polyfill' of https://github.com/prateekbh/amphtml i…

c4d3957

…nto fet-polyfill

removing extra test

1015cdd

jridgewell reviewed Aug 14, 2018

View reviewed changes

prateekbh added 2 commits August 14, 2018 13:13

response as a class

ca2b809

resolving comments

75483d3

jridgewell reviewed Aug 15, 2018

View reviewed changes

prateekbh commented Aug 15, 2018

View reviewed changes

fixing type related issues

e1456e3

prateekbh force-pushed the fet-polyfill branch from 4ed1c4b to e1456e3 Compare August 15, 2018 21:26

jridgewell approved these changes Aug 17, 2018

View reviewed changes

prateekbh added 2 commits August 20, 2018 08:27

renaming file to avoid test conflict

4644f74

adding copyright

6135481

prateekbh merged commit 43f9a86 into ampproject:master Aug 20, 2018

prateekbh deleted the fet-polyfill branch August 20, 2018 19:29

Fetch polyfill #17468

Fetch polyfill #17468

Conversation

prateekbh commented Aug 13, 2018

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

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

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

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

prateekbh 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

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