Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write local testing config values to AMP runtime #12152

Merged
merged 4 commits into from
Nov 21, 2017
Merged

Write local testing config values to AMP runtime #12152

merged 4 commits into from
Nov 21, 2017

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Nov 21, 2017

This PR does the following:

  1. Makes sure that the values of localDev: true, thirdPartyFrameHost, and thirdPartyFrameRegex are written to the local AMP config to enable local testing with gulp build and gulp dist --fortesting.
  2. Adds a new parameter --local_dev to gulp prepend-global, which will ensure that the AMP runtime is set up for local development and testing.

With this, localDev will be set to true when the runtime is built and served on localhost via gulp, or via the following commands:

gulp clean
gulp build
gulp serve

In addition, localDev will be set to true and thirdPartyFrameHost and thirdPartyFrameRegex will point to AMP_TESTING_HOST when the runtime is built and served from a non-localhost server, via the following commands.

gulp clean
export AMP_TESTING_HOST=<hostname.domain.com>
gulp build
gulp serve --host=<hostname.domain.com>

Partial fix for #12054

@rsimha
Copy link
Contributor Author

rsimha commented Nov 21, 2017

/to @erwinmombay @bradfrizzell for review.

* @param {string} opt_local
* @param {string=} opt_branch
* @param {string} filename File containing the config
* @param {string} opt_localBranch Whether to use the local branch version
Copy link
Member

Choose a reason for hiding this comment

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

if this is actually opt then add =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* @param {string} config Prod or canary
* @param {string} target File containing the AMP runtime (amp.js or v0.js)
* @param {string} filename File containing the (prod or canary) config
* @param {boolean} opt_localDev Whether to enable local development
Copy link
Member

Choose a reason for hiding this comment

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

missing = on these optional params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@rsimha
Copy link
Contributor Author

rsimha commented Nov 21, 2017

@erwinmombay Will merge this shortly, unless you have more comments.

@rsimha rsimha merged commit cc87336 into ampproject:master Nov 21, 2017
@rsimha rsimha deleted the 2017-11-20-Localhost branch November 21, 2017 20:46
ghost pushed a commit that referenced this pull request Dec 6, 2017
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants