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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

馃彈 Apply AMP_CONFIG to runtime files during gulp dist #26554

Merged
merged 3 commits into from Feb 3, 2020
Merged

馃彈 Apply AMP_CONFIG to runtime files during gulp dist #26554

merged 3 commits into from Feb 3, 2020

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Jan 29, 2020 •
edited

This PR updates gulp dist to generate production-ready AMP binaries by default.

Highlights:

  • Today, gulp dist doesn't add AMP_CONFIG to runtime files.
    • With this PR, gulp dist will add AMP_CONFIG with prod flags.
    • The values of localDev and test in AMP_CONFIG will be false.
  • The old behavior of gulp dist can be triggered by running gulp dist --noconfig.
  • The behavior of gulp dist --fortesting remains unchanged (AMP_CONFIG is added, localDev and test are true).
  • The behavior of gulp build [--fortesting] remains unchanged (AMP_CONFIG is added, localDev is true, test is based on argv.fortesting).

Fixes #24817

@rsimha rsimha self-assigned this Jan 29, 2020
@rsimha
Copy link
Contributor Author

rsimha commented Jan 29, 2020 •
edited

/to @jridgewell, @rcebulko, @danielrozenberg (review)
/cc @choumx, @ssantosms, @erwinmombay as FYI

rcebulko
rcebulko reviewed Jan 30, 2020
View reviewed changes
Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

Do devs need to be alerted about this/will this alter workflow for people?

build-system/tasks/helpers.js Show resolved Hide resolved
@rsimha
Copy link
Contributor Author

rsimha commented Jan 30, 2020

Do devs need to be alerted about this/will this alter workflow for people?

Good question. The default local development workflow uses --fortesting, and that behavior remains unchanged. This PR addresses a corner-case for AMP developers, and a specific need from one of our partners.

I've added a new commit that updates our documentation. I will mention this in our fortnightly updates. And our partner is aware that this change is forthcoming.

@rsimha
Copy link
Contributor Author

rsimha commented Feb 3, 2020

Bump. This still needs an approving review.

@rsimha rsimha merged commit 53cb630 into ampproject:master Feb 3, 2020
@rsimha rsimha deleted the 2020-10-29-DistConfig branch February 3, 2020 19:11
erwinmombay pushed a commit that referenced this pull request Feb 11, 2020
@rsimha @erwinmombay
馃彈 Apply AMP_CONFIG to runtime files during gulp dist (#26554)
1f60ce1
erwinmombay pushed a commit that referenced this pull request Feb 18, 2020
@rsimha @erwinmombay
馃彈 Apply AMP_CONFIG to runtime files during gulp dist (#26554)
49e0161
@rsimha rsimha mentioned this pull request May 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielrozenberg danielrozenberg danielrozenberg left review comments

@rcebulko rcebulko rcebulko approved these changes

@dreamofabear dreamofabear Awaiting requested review from dreamofabear

@erwinmombay erwinmombay Awaiting requested review from erwinmombay

@jridgewell jridgewell Awaiting requested review from jridgewell

Assignees

@rsimha rsimha

Labels
cla: yes PR use: In Beta / Experimental PR use: In Stable WG: infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gulp dist doesn't prepend experiments by default
5 participants
@rsimha @danielrozenberg @rcebulko @googlebot @ampprojectbot