Moving AMP to fetch polyfill #17642

prateekbh · 2018-08-21T21:46:49Z

Utilizes fetch polyfill.
Deletes all instances of FetchResponse and FetchPolyfill

…ove-to-fetch

…into move-to-fetch

…ove-to-fetch

prateekbh · 2018-08-31T22:16:16Z

src/document-fetcher.js

+        const options = {
+          status: xhr.status,
+          statusText: xhr.statusText,
+          headers: parseHeaders(xhr.getAllResponseHeaders()),


@jridgewell, we have a pending discussion around this piece of code in one of the earlier PRs, where you wanted to skip this part.

If we want to skip this we'll need to change code in assetSuccess and verifyAmpCorsHeaders code as they both rely on Response type

jridgewell · 2018-08-31T17:41:59Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

@@ -688,9 +688,9 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
    this.ampAnalyticsConfig_ = extractAmpAnalyticsConfig(this, responseHeaders);
    this.qqid_ = responseHeaders.get(QQID_HEADER);
    this.troubleshootData_.creativeId =
-        responseHeaders.get('google-creative-id');
+        (/** @type {string} **/ (responseHeaders.get('google-creative-id')));


dev().assertString()

question: there is no guarantee that this'll be a string, but the code consuming this assumed so. Adding a dev assert might just start throwing errors which aren't expected.

In other words this might have been null in some cases even today, but never caught. Adding a dev assert might start surfacing this. It is a great thing to have if we WANT to catch these, but not if we want to ignore these.

Not sure about the use case here.

pinged author, seems like dev assert will be fine

jridgewell · 2018-08-31T17:42:06Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

    this.troubleshootData_.lineItemId =
-        responseHeaders.get('google-lineitem-id');
+        (/** @type {string} **/ (responseHeaders.get('google-lineitem-id')));


jridgewell · 2018-08-31T17:47:52Z

src/document-fetcher.js

+  // Replace instances of \r\n and \n followed by at least one space or
+  // horizontal tab with a space.
+  // https://tools.ietf.org/html/rfc7230#section-3.2
+  const preProcessedHeaders = rawHeaders.replace(/\r?\n[\t ]+/g, ' ');


This isn't explained in the link.

jridgewell · 2018-09-04T18:34:20Z

extensions/amp-a4a/0.1/signature-verifier.js

@@ -315,7 +315,8 @@ export class SignatureVerifier {
                  'Fast Fetch keyset spec requires Content-Type: ' +
                      'application/jwk-set+json');
              return response.json().then(


Can we override the definition for Response.p.json to return a !JsonObject?

I have no idea no to do this.
can you explain a bit?

I don't either. I just don't want to always /** @type {!JsonObject} */ when we call Reponse.p.json.

How does dev.assertJson sound?

jridgewell · 2018-09-04T18:34:30Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

@@ -716,7 +716,7 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
    if (responseHeaders.get('amp-ff-pageview-tokens')) {
      this.removePageviewStateToken();
      this.setPageviewStateToken(
-          responseHeaders.get('amp-ff-pageview-tokens'));
+          /** @type {string} */ (responseHeaders.get('amp-ff-pageview-tokens')));


jridgewell · 2018-09-04T18:38:56Z

src/document-fetcher.js

+  // https://tools.ietf.org/html/rfc7230#section-3.2
+  const preProcessedHeaders = rawHeaders.replace(/\r?\n[\t ]+/g, ' ');
+  preProcessedHeaders.split(/\r?\n/).forEach(function(line) {
+    const parts = line.split(':');


const match = /([^:]+):(.*)/.exec(line);

…ove-to-fetch

prateekbh · 2018-09-05T19:38:32Z

adding @glevitzky to review the default value for headers in amp-ad-network-doubleclick-impl.js

…ove-to-fetch

* moving to fetch polyfills * dep-check fixes * fixing batched-xhr tests * fixing more tests * fixing xhr-tests * fixing amp-ad-tests * fixing more tests * Update test-doubleclick-sra.js * Update test-doubleclick-sra.js * Update test-doubleclick-sra.js * adding proxy methods * fixing document-fetcher * document-fetcher * resolving comments * adds a default value * fixing fetch value * increasing bundlesize for fetch polyfill * Update bundle-size.js * missed the 0.1kb warning

moving to fetch polyfills

700514a

googlebot added the cla: yes label Aug 21, 2018

prateekbh added 2 commits August 21, 2018 15:03

dep-check fixes

b594869

fixing batched-xhr tests

cd48b33

prateekbh changed the title ~~moving AMP to fetch polyfill~~ [WIP] moving AMP to fetch polyfill Aug 21, 2018

prateekbh added 15 commits August 21, 2018 17:03

fixing more tests

ff3de93

fixing xhr-tests

a48aa48

fixing amp-ad-tests

273ace0

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

7ca7962

…ove-to-fetch

fixing more tests

7e56851

Update test-doubleclick-sra.js

91d06a5

Update test-doubleclick-sra.js

1955c9f

Update test-doubleclick-sra.js

323b0d7

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

028b46e

…ove-to-fetch

Merge branch 'move-to-fetch' of https://github.com/prateekbh/amphtml …

94b274f

…into move-to-fetch

adding proxy methods

38dcb63

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

4c9ea81

…ove-to-fetch

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

7aaeb8a

…ove-to-fetch

fixing document-fetcher

b15af56

document-fetcher

c0b564f

prateekbh requested a review from jridgewell August 31, 2018 10:17

prateekbh changed the title ~~[WIP] moving AMP to fetch polyfill~~ Moving AMP to fetch polyfill Aug 31, 2018

prateekbh commented Aug 31, 2018

View reviewed changes

jridgewell reviewed Sep 4, 2018

View reviewed changes

prateekbh added 2 commits September 5, 2018 10:16

resolving comments

25c0f25

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

7541d67

…ove-to-fetch

jridgewell approved these changes Sep 5, 2018

View reviewed changes

adds a default value

b28bed6

prateekbh requested a review from glevitzky September 5, 2018 19:38

glevitzky approved these changes Sep 6, 2018

View reviewed changes

prateekbh added 8 commits September 6, 2018 11:36

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

0384afc

…ove-to-fetch

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

867a4d7

…ove-to-fetch

fixing fetch value

8417bc1

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

27e2d07

…ove-to-fetch

increasing bundlesize for fetch polyfill

bf25a05

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

c97737a

…ove-to-fetch

Update bundle-size.js

58ee304

missed the 0.1kb warning

27c18fc

prateekbh merged commit bc2b84f into ampproject:master Sep 17, 2018

prateekbh deleted the move-to-fetch branch September 17, 2018 19:51

prateekbh mentioned this pull request Nov 28, 2018

Move AMP runtime to standard fetch #16707

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving AMP to fetch polyfill #17642

Moving AMP to fetch polyfill #17642

prateekbh commented Aug 21, 2018 •

edited

Loading

prateekbh Aug 31, 2018

jridgewell Aug 31, 2018

prateekbh Sep 4, 2018 •

edited

Loading

prateekbh Sep 5, 2018

jridgewell Aug 31, 2018

jridgewell Aug 31, 2018

prateekbh Sep 5, 2018

jridgewell Sep 4, 2018

prateekbh Sep 5, 2018

jridgewell Sep 5, 2018

prateekbh Sep 5, 2018

jridgewell Sep 4, 2018

prateekbh Sep 5, 2018

jridgewell Sep 4, 2018

prateekbh commented Sep 5, 2018 •

edited

Loading

Moving AMP to fetch polyfill #17642

Moving AMP to fetch polyfill #17642

Conversation

prateekbh commented Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekbh Sep 4, 2018 • 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

prateekbh commented Sep 5, 2018 • edited Loading

prateekbh commented Aug 21, 2018 •

edited

Loading

prateekbh Sep 4, 2018 •

edited

Loading

prateekbh commented Sep 5, 2018 •

edited

Loading