Skip to content

Reenable EsbuildCompilation on EXP_C#36105

Merged
samouri merged 5 commits intoampproject:mainfrom
samouri:enable-esbuild
Sep 23, 2021
Merged

Reenable EsbuildCompilation on EXP_C#36105
samouri merged 5 commits intoampproject:mainfrom
samouri:enable-esbuild

Conversation

@samouri
Copy link
Copy Markdown
Member

@samouri samouri commented Sep 17, 2021

summary
See #35816 for a PR that forces esbuild to be true and has all of CI green. Adds in one important fix for EXP_C, and then two fixes that only matter when it is the primary bundler:

  1. (important): accept either unminified or minified name for getAmpConfigForFile. The distinction wasn't useful and forced us to pass in the minified name for minified builds (srcFilename is more ergonomic). Instead I pass destFilename
  2. visual-diff:test:true check should also allow !0 since that is the minified version of true
  3. check-sourcemaps: esbuild has first line of the output on the first line instead of the second.

confidence checks

  • prop mangling has been disabled for now. this is the riskiest optimization we could do, and my goal now is to create a baseline of how this branch performs without it. If equivalent to the closure version, we can then pursue mangling separately.
  • all automated tests pass
  • manual testing of many sites (ping for details)
  • full manual qa pass

@samouri samouri self-assigned this Sep 17, 2021
@samouri samouri changed the title Revert "Disable EsbuildCompilation experiment. (#35814)" Reenable EsbuildCompilation on EXP_C Sep 17, 2021
@samouri samouri marked this pull request as ready for review September 20, 2021 19:24
@amp-owners-bot amp-owners-bot bot requested a review from rsimha September 20, 2021 19:24
@samouri samouri requested a review from jridgewell September 20, 2021 19:24
@amp-owners-bot
Copy link
Copy Markdown

Hey @danielrozenberg! These files were changed:

build-system/tasks/visual-diff/index.js

@samouri samouri force-pushed the enable-esbuild branch 2 times, most recently from 76b3762 to de5c258 Compare September 22, 2021 15:14
@samouri
Copy link
Copy Markdown
Member Author

samouri commented Sep 22, 2021

@rsimha: PTAL

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.

LGTM after a minor readability fix.

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.

4 participants