Skip to content

Revert "Revert "Improve AMP_CONFIG handling during build (#35773)" (#35819)"#36008

Merged
samouri merged 2 commits intoampproject:mainfrom
samouri:revert-35819-revert-35773-better-prepend
Sep 17, 2021
Merged

Revert "Revert "Improve AMP_CONFIG handling during build (#35773)" (#35819)"#36008
samouri merged 2 commits intoampproject:mainfrom
samouri:revert-35819-revert-35773-better-prepend

Conversation

@samouri
Copy link
Copy Markdown
Member

@samouri samouri commented Sep 9, 2021 •
edited
Loading

summary
See Attempt 1 for full PR description: #35773

In the prior attempt, the npx amp command was not setting localDev: true for the amp build and amp default task commands. This caused all extension loading to default to cdn instead of localhost. This should be fixed via 3d1d2fe. Note: this PR is also important for migrating to a different bundler, so that we can separate out the notion of bundler being directly tied to mode. Before this PR, closure-->(localDev=false), esbuild-->(localDev=true). After the PR they are tied to the build command instead (build, dist etc)

testing done

  • All CI passes
  • Verified that npx amp, npx amp build, and npx amp dist all have identical AMP_CONFIG's to their counterparts on main. Technically npx amp dist has a slightly different output, but I verified it is functionally the same. The config is now minified via terser in the closure flow, and therefore values like 0.01 --> .01

@amp-owners-bot amp-owners-bot bot requested a review from dvoytenko September 9, 2021 03:56
@samouri samouri mentioned this pull request Sep 9, 2021
Closed
@samouri samouri force-pushed the revert-35819-revert-35773-better-prepend branch 2 times, most recently from becc138 to c9b74bb Compare September 9, 2021 03:58
@samouri samouri changed the base branch from revert-35819-revert-35773-better-prepend to main September 9, 2021 04:01
@samouri samouri changed the title Revert 35819 revert 35773 better prepend Revert "Revert "Improve AMP_CONFIG handling during build (#35773)" (#35819)" Sep 9, 2021
@ampproject ampproject deleted a comment from amp-owners-bot bot Sep 9, 2021
@samouri samouri removed the request for review from dvoytenko September 9, 2021 04:02
@samouri samouri marked this pull request as draft September 9, 2021 04:02
@samouri samouri force-pushed the revert-35819-revert-35773-better-prepend branch from 6230f5a to 5adb698 Compare September 9, 2021 04:23
@samouri samouri self-assigned this Sep 9, 2021
@samouri samouri force-pushed the revert-35819-revert-35773-better-prepend branch from 5adb698 to 3d1d2fe Compare September 9, 2021 04:32
@samouri samouri marked this pull request as ready for review September 10, 2021 15:37
@amp-owners-bot amp-owners-bot bot requested a review from rsimha September 10, 2021 15:37
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Sep 10, 2021

Hey @rsimha! These files were changed:

build-system/compile/compile.js
build-system/pr-check/module-tests.js
build-system/pr-check/nomodule-tests.js
build-system/server/lazy-build.js

@samouri samouri force-pushed the revert-35819-revert-35773-better-prepend branch from 3d1d2fe to 4347d12 Compare September 10, 2021 15:39
@samouri samouri requested a review from jridgewell September 10, 2021 17:10
@samouri samouri force-pushed the revert-35819-revert-35773-better-prepend branch from 4347d12 to d08f6ee Compare September 14, 2021 20:26
rsimha
rsimha approved these changes Sep 16, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

👏 👏 👏

build-system/compile/post-closure-babel.js
Comment on lines -38 to -50
const basename = path.basename(filename, argv.esm ? '.mjs' : '.js');
if (!mainBundles) {
mainBundles = Object.keys(jsBundles).map((key) => {
const bundle = jsBundles[key];
if (bundle.options && bundle.options.minifiedName) {
return path.basename(bundle.options.minifiedName, '.js');
}
return path.basename(key, '.js');
});
}
if (mainBundles.includes(basename)) {
options.output.preamble = ';';
}
Copy link
Copy Markdown
Contributor

@rsimha rsimha Sep 16, 2021

Choose a reason for hiding this comment

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

Glad to see this (hacky?) block go away.

Copy link
Copy Markdown
Member Author

@samouri samouri Sep 17, 2021

Choose a reason for hiding this comment

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

Hopefully gone for good this time! 🤞

build-system/tasks/prepend-global/index.js
sanityCheck(contents);
const config = /self\.AMP_CONFIG\|\|\(self\.AMP_CONFIG=.*?\/\*AMP_CONFIG\*\//;
const config =
/self\.AMP_CONFIG\|\|\(self\.AMP_CONFIG=(.|\n)*?\/\*AMP_CONFIG\*\//;
Copy link
Copy Markdown
Contributor

@rsimha rsimha Sep 16, 2021

Choose a reason for hiding this comment

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

To prevent bugs from future maintainers, can we add a one-liner explainer comment for this regex?

Copy link
Copy Markdown
Member Author

@samouri samouri Sep 17, 2021

Choose a reason for hiding this comment

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

Sure! I believe this regex is simply for removing the AMP_CONFIG from the start of the file

Copy link
Copy Markdown
Member Author

@samouri samouri Sep 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I don't think a 1-liner will help with this one since it is helpfully within a self-documenting fn named removeConfig, so I think the purpose is relatively clear.

The actual mechanism could use some documenting. I'll do that in a followup PR, as there are certain other changes I'd like to try to make before EOD that I don't want to block on!

@samouri samouri merged commit 54b886f into ampproject:main Sep 17, 2021
@samouri samouri mentioned this pull request Sep 24, 2021
Closed
@jridgewell jridgewell mentioned this pull request Feb 11, 2022
Merged
@samouri samouri deleted the revert-35819-revert-35773-better-prepend branch February 11, 2022 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jridgewell jridgewell jridgewell approved these changes

+1 more reviewer

@rsimha rsimha rsimha approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@samouri samouri

Labels

PR use: In LTS PR use: In Stable WG: infra WG: performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@samouri @jridgewell @rsimha @ampprojectbot