Allow for hosted testing to override where third party frame is retrieved from #5890

erwinmombay · 2016-10-28T21:04:19Z

erwinmombay · 2016-10-28T22:28:24Z

src/mode.js

+const thirdPartyFrameHost = urls.thirdPartyFrameHost;
+
+/** @type {boolean} */
+const urlsForceLocalDev = !!urls.localDev;


@cramforce this increases file sizes a little bit, hoping it goes away with the mode optimization.

erwinmombay · 2016-10-28T22:28:50Z

src/config.js

  cdn: env['cdnUrl'] || 'https://cdn.ampproject.org',
  errorReporting: env['errorReportingUrl'] ||
      'https://amp-error-reporting.appspot.com/r',
+  localDev: true


@cramforce any other better way to figure out that its not localhost but treat the environment as localDev ?

erwinmombay · 2016-10-28T22:29:05Z

src/config.js

  thirdPartyFrameRegex: env['thirdPartyFrameRegex'] ||
-      /^d-\d+\.ampproject\.net$/,
+      /erwinm\.herokuapp\.com/,


this file will be reverted. just displaying for review and testing

erwinmombay · 2016-10-28T22:29:24Z

see http://erwinm.herokuapp.com/examples/ads.amp.max.html and http://erwinm.herokuapp.com/examples/ads.amp.min.html for working ads hosted on heroku server

cramforce · 2016-10-28T22:52:37Z

DEVELOPING.md

@@ -127,6 +127,31 @@ For deploying and testing local AMP builds on [HEROKU](https://www.heroku.com/)

 Meantime, you can also use our automatic build on Heroku [link](http://amphtml-nightly.herokuapp.com/), which is normally built with latest head on master branch (please allow delay). The first time load is normally slow due to Heroku's free account throttling policy.

+To be able to test ads and third party frame make sure to `thirdPartyFrameHost`


This is not what I was hoping we'd do.

Couldn't we automatically generate the right config for localDev and heroku and pre-prepend it using the AMP_CONFIG mechanism?

yes, we could certainly do that

ill make the changes in the build system

erwinmombay · 2016-10-31T16:02:52Z

package.json

+    "heroku-postbuild": "gulp clean && gulp build --fortesting && gulp dist --fortesting && npm run try-prepend",
+    "try-prepend": "if [ -n \"$AMP_TESTING_HOST\" ]; then npm run prepend-amp; npm run prepend-v0; fi",
+    "prepend-amp": "gulp prepend-global --prod node_modules/AMP_CONFIG.json --local --target dist/amp.js && gulp prepend-global --prod node_modules/AMP_CONFIG.json --local --target dist.3p/current/integration.js",
+    "prepend-v0": "gulp prepend-global --prod node_modules/AMP_CONFIG.json --local --target dist/v0.js && gulp prepend-global --prod node_modules/AMP_CONFIG.json --local --target dist.3p/current-min/f.js"


@cramforce made the changes.

I did realize though that the AMP_CONFIG url overriding doesn't work in production right now cause we never prepend the global configs to f.js. should we be prepending it?

cc @dknecht

sorry, meant to clarify. on cdn.ampproject.org we don't need to do the prepend f.js since the values we want are the default but for any other cache they would need to prepend the AMP_CONFIG to f.js, so just an update to the SERVING.md file is required

erwinmombay · 2016-10-31T21:36:51Z

@dknecht could you review this or have somebody review it?

@zhouyx do you mind also giving this a pass? thanks

zhouyx · 2016-10-31T21:55:10Z

DEVELOPING.md

@@ -127,6 +128,10 @@ For deploying and testing local AMP builds on [HEROKU](https://www.heroku.com/)

 Meantime, you can also use our automatic build on Heroku [link](http://amphtml-nightly.herokuapp.com/), which is normally built with latest head on master branch (please allow delay). The first time load is normally slow due to Heroku's free account throttling policy.

+To correctly get ads and third party working when testing on hosted services
+you will need to a `AMP_TESTING_HOST` environment variable. (On heroku this is


zhouyx · 2016-10-31T22:06:01Z

build-system/SERVING.md

@@ -52,6 +52,7 @@ rm /path/to/cdn/production/alp.js.bak
 # make sure and prepend the global production config to main binaries
 gulp prepend-global --target /path/to/cdn/production/v0.js --prod
 gulp prepend-global --target /path/to/cdn/production/alp.js --prod
+gulp prepend-global --target /path/to/3p/cdn/production/f.js --prod


every 3p has a f.js?

should only be 1, in our case i mean the .ampproject.net/ host

cramforce

This is great. One question.

cramforce · 2016-10-31T22:25:27Z

package.json

@@ -19,7 +19,10 @@
    "lint": "gulp lint",
    "build": "gulp build",
    "dist": "gulp dist",
-    "heroku-postbuild": "gulp clean && gulp build && gulp dist --fortesting"
+    "heroku-postbuild": "gulp clean && gulp build --fortesting && gulp dist --fortesting && npm run try-prepend",
+    "try-prepend": "if [ -n \"$AMP_TESTING_HOST\" ]; then npm run prepend-amp; npm run prepend-v0; fi",


Can we just fail if this is not present with a message what to do?

done. it is a bit noisy though cause it throws a bunch of npm errors right after. worried it might not be too visible

erwinmombay · 2016-10-31T23:11:08Z

build-system/SERVING.md

@@ -52,6 +52,7 @@ rm /path/to/cdn/production/alp.js.bak
 # make sure and prepend the global production config to main binaries
 gulp prepend-global --target /path/to/cdn/production/v0.js --prod
 gulp prepend-global --target /path/to/cdn/production/alp.js --prod
+gulp prepend-global --target /path/to/3p/cdn/production/f.js --prod


@cramforce are we ok with recommending this? we dont need to do it for our f.js but it definitely needs to be done for other caches

Yeah, sure.

…eved from

erwinmombay · 2016-11-02T08:43:57Z

@zhouyx @cramforce PTAL. still need approval

cramforce · 2016-11-02T13:58:48Z

package.json

@@ -19,7 +19,10 @@
    "lint": "gulp lint",
    "build": "gulp build",
    "dist": "gulp dist",
-    "heroku-postbuild": "gulp clean && gulp build && gulp dist --fortesting"
+    "heroku-postbuild": "npm run test-env-var && gulp clean && gulp build --fortesting && gulp dist --fortesting && npm run prepend-amp && npm run prepend-v0",
+    "test-env-var": "if [ -z \"${AMP_TESTING_HOST}\" ]; then echo \"ERROR: Please set the 'AMP_TESTING_HOST' environment variable. If you are in heroku this can be set by executing 'heroku config:set AMP_TESTING_HOST=my-heroku-subdomain.herokuapp.com' on the command line.\"; exit 1; fi",


Another question: Does heroku expose and environment var that we could use as a default?

ran heroku run printenv and I don't see one we could use

…tion rewrite (ampproject#5982) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples Introduce amp-auto-id attribute to AMP element. (ampproject#5973) * Introduce amp-auto-id attribute to AMP element. * Fix tests * Address comments * Address comments * revert amp-live-list.md polling clarification (ampproject#5991) Only report 1% of errors if a page has non-AMP JS (ampproject#5994) Fixes ampproject#5732 Allow for hosted testing to override where third party frame is retrieved from (ampproject#5890) * Allow for hosted testing to override where third party frame is retrieved from * test my own site * fix ava tests Remove unsupported query selector feature (ampproject#5999) Workaround for misbehaving webview viewer (ampproject#6001) A webview viewer from Google is seing origins of the form `www.google.com`. For the time being we accept these as they trigger errors in our error reporting that aren't useful. As this form of origins can only occur with webviews that have full control over the page via JS injection anyway, this change should make no difference for normal web usage. A4A integration tests (ampproject#5812) * First draft of full integration tests for A4A. * First draft of full integration tests for A4A. * Got tests of exception conditions working. Refactored some common stuff to utils.js. * Refactored XDom and friendly iframe tests to helpers. * Added tests. * Resolved merge conflicts. * Centralized visibility check and updated visibility test. * Moved all A4A tests to using central visibility test. * Lint fixes. * Changed custom expectations to local functions. * Replaced stringToArrayBuffer with utf8Encode. * Fixed visibility assertion. * Lint fixes. * Added a comment (partly in order to get Travis to re-run). * Changes in response to reviews. * Rebased and fixed small rebase error. Fixes an issue where ads are not correctly centered on certain platforms. (ampproject#6003) * Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property. * Added check for existence and changed to plain old 'transform'. * Now asserting the existence of the container. amp-sticky-ad close button new style (ampproject#5979) * unit style * fix css * fix test * rebase * fix z-index Other JS errors: Use startsWith (ampproject#6006) Fixes ampproject#5994 (comment). Fix log calls without TAGNAME (ampproject#6005) My presubmit was a bit too lenient, and too strict at the same time. refactor amp-ad.css (ampproject#5992) Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 (ampproject#5576) * Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 * Updated non-supported vars + format changes for amp-analytics vars * Updates per Avi's feedback * Updated descriptions for amp-carousel vars add version parameter to AMP.extension signature (ampproject#5989) * add version parameter to AMP.extension signature * apply recommendations * remove TODO Improve error reporting (ampproject#6019) - Include the current `location.hash`. We are missing it, because it is not included in the referrer. - Include previously recorded errors, so it is easier to identify follow on errors. - Kill the 2000 char limit as it isn't important for AMP's target browsers. Include the originalHash (ampproject#6020) We remove the hash in embedding mode, so the original code doesn't actually work. Fix error: "Can't find variable: TextDecoder" (ampproject#6011) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples * Include self (window) when verifying TextDecoder/TextEncoder existence * fix upstream conflict * PR feedback * PR feedback

Update ads.amp.html Modify A4A AMP Creative to use ampRuntimeUtf16CharOffsets from validation rewrite (ampproject#5982) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples Introduce amp-auto-id attribute to AMP element. (ampproject#5973) * Introduce amp-auto-id attribute to AMP element. * Fix tests * Address comments * Address comments * revert amp-live-list.md polling clarification (ampproject#5991) Only report 1% of errors if a page has non-AMP JS (ampproject#5994) Fixes ampproject#5732 Allow for hosted testing to override where third party frame is retrieved from (ampproject#5890) * Allow for hosted testing to override where third party frame is retrieved from * test my own site * fix ava tests Remove unsupported query selector feature (ampproject#5999) Workaround for misbehaving webview viewer (ampproject#6001) A webview viewer from Google is seing origins of the form `www.google.com`. For the time being we accept these as they trigger errors in our error reporting that aren't useful. As this form of origins can only occur with webviews that have full control over the page via JS injection anyway, this change should make no difference for normal web usage. A4A integration tests (ampproject#5812) * First draft of full integration tests for A4A. * First draft of full integration tests for A4A. * Got tests of exception conditions working. Refactored some common stuff to utils.js. * Refactored XDom and friendly iframe tests to helpers. * Added tests. * Resolved merge conflicts. * Centralized visibility check and updated visibility test. * Moved all A4A tests to using central visibility test. * Lint fixes. * Changed custom expectations to local functions. * Replaced stringToArrayBuffer with utf8Encode. * Fixed visibility assertion. * Lint fixes. * Added a comment (partly in order to get Travis to re-run). * Changes in response to reviews. * Rebased and fixed small rebase error. Fixes an issue where ads are not correctly centered on certain platforms. (ampproject#6003) * Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property. * Added check for existence and changed to plain old 'transform'. * Now asserting the existence of the container. amp-sticky-ad close button new style (ampproject#5979) * unit style * fix css * fix test * rebase * fix z-index Other JS errors: Use startsWith (ampproject#6006) Fixes ampproject#5994 (comment). Fix log calls without TAGNAME (ampproject#6005) My presubmit was a bit too lenient, and too strict at the same time. refactor amp-ad.css (ampproject#5992) Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 (ampproject#5576) * Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 * Updated non-supported vars + format changes for amp-analytics vars * Updates per Avi's feedback * Updated descriptions for amp-carousel vars add version parameter to AMP.extension signature (ampproject#5989) * add version parameter to AMP.extension signature * apply recommendations * remove TODO Improve error reporting (ampproject#6019) - Include the current `location.hash`. We are missing it, because it is not included in the referrer. - Include previously recorded errors, so it is easier to identify follow on errors. - Kill the 2000 char limit as it isn't important for AMP's target browsers. Include the originalHash (ampproject#6020) We remove the hash in embedding mode, so the original code doesn't actually work. Fix error: "Can't find variable: TextDecoder" (ampproject#6011) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples * Include self (window) when verifying TextDecoder/TextEncoder existence * fix upstream conflict * PR feedback * PR feedback

…eved from (ampproject#5890) * Allow for hosted testing to override where third party frame is retrieved from * test my own site * fix ava tests

erwinmombay force-pushed the fix-heroku-setup branch from bb29e80 to fc579ad Compare October 28, 2016 21:16

erwinmombay assigned cramforce Oct 28, 2016

erwinmombay commented Oct 28, 2016

View reviewed changes

cramforce reviewed Oct 28, 2016

View reviewed changes

erwinmombay force-pushed the fix-heroku-setup branch 4 times, most recently from 28e687f to 661a5e3 Compare October 31, 2016 16:01

erwinmombay commented Oct 31, 2016

View reviewed changes

erwinmombay mentioned this pull request Oct 31, 2016

Meeting Notes 2016-10-31 #5926

Closed

erwinmombay assigned zhouyx Oct 31, 2016

erwinmombay force-pushed the fix-heroku-setup branch from 661a5e3 to bd8913c Compare October 31, 2016 21:41

zhouyx reviewed Oct 31, 2016

View reviewed changes

erwinmombay force-pushed the fix-heroku-setup branch from bd8913c to d02d4d4 Compare October 31, 2016 22:15

cramforce reviewed Oct 31, 2016

View reviewed changes

erwinmombay force-pushed the fix-heroku-setup branch from d02d4d4 to 596e75a Compare October 31, 2016 23:07

erwinmombay commented Oct 31, 2016

View reviewed changes

erwinmombay force-pushed the fix-heroku-setup branch 3 times, most recently from da95832 to 946a9f3 Compare November 1, 2016 22:25

erwinmombay added 3 commits November 2, 2016 00:36

Allow for hosted testing to override where third party frame is retri…

7e132dc

…eved from

test my own site

a31fdf0

fix ava tests

bc92fae

erwinmombay force-pushed the fix-heroku-setup branch from 946a9f3 to bc92fae Compare November 2, 2016 08:11

cramforce reviewed Nov 2, 2016

View reviewed changes

cramforce approved these changes Nov 3, 2016

View reviewed changes

erwinmombay merged commit b65f168 into ampproject:master Nov 3, 2016

erwinmombay mentioned this pull request Nov 7, 2016

Meeting Notes 2016-11-07 #6067

Closed

erwinmombay mentioned this pull request Jan 12, 2017

make --fortesting flag generate a binary that points to localhost as the bootstrap base url #3166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for hosted testing to override where third party frame is retrieved from #5890

Allow for hosted testing to override where third party frame is retrieved from #5890

erwinmombay commented Oct 28, 2016 •

edited

erwinmombay Oct 28, 2016

erwinmombay Oct 28, 2016

erwinmombay Oct 28, 2016

erwinmombay commented Oct 28, 2016

cramforce Oct 28, 2016

erwinmombay Oct 28, 2016

erwinmombay Oct 28, 2016

erwinmombay Oct 31, 2016

erwinmombay Oct 31, 2016

erwinmombay commented Oct 31, 2016

zhouyx Oct 31, 2016

erwinmombay Oct 31, 2016

zhouyx Oct 31, 2016

erwinmombay Oct 31, 2016

cramforce left a comment

cramforce Oct 31, 2016

erwinmombay Oct 31, 2016

erwinmombay Oct 31, 2016

cramforce Oct 31, 2016

erwinmombay commented Nov 2, 2016

cramforce Nov 2, 2016

erwinmombay Nov 2, 2016

erwinmombay Nov 3, 2016

		@@ -127,6 +127,31 @@ For deploying and testing local AMP builds on [HEROKU](https://www.heroku.com/)

		Meantime, you can also use our automatic build on Heroku [link](http://amphtml-nightly.herokuapp.com/), which is normally built with latest head on master branch (please allow delay). The first time load is normally slow due to Heroku's free account throttling policy.

		To be able to test ads and third party frame make sure to `thirdPartyFrameHost`

Allow for hosted testing to override where third party frame is retrieved from #5890

Allow for hosted testing to override where third party frame is retrieved from #5890

Conversation

erwinmombay commented Oct 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay commented Oct 28, 2016

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

erwinmombay commented Oct 31, 2016

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

cramforce 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

erwinmombay commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay commented Oct 28, 2016 •

edited