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

Reenable EsbuildCompilation on EXP_C #36105

Merged
merged 5 commits into from
Sep 23, 2021
Merged

Conversation

samouri
Copy link
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
Copy link

Hey @danielrozenberg! These files were changed:

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

build-system/tasks/check-sourcemaps.js Outdated Show resolved Hide resolved
build-system/tasks/prepend-global/index.js Outdated Show resolved Hide resolved
@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
Member Author

samouri commented Sep 22, 2021

@rsimha: PTAL

Copy link
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.

build-system/tasks/check-sourcemaps.js Outdated Show resolved Hide resolved
build-system/tasks/helpers.js Show resolved Hide resolved
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