🚮 Remove closure compiler logic from build-system. by samouri · Pull Request #37296 · ampproject/amphtml

samouri · 2022-01-04T22:22:24Z

summary
Deletes a files directly related to closure compilation from build-system.

Future related changes:

Remove dep on closure from validator task, s.t. we can remove the dep from package.json
Remove externs.
Remove rrule hack / dep patches.
Remove jsdoc-isms that only applied to closure and have no value outside of that (closurePrimitive)
Remove babel passes that only applied to closure

amp-owners-bot · 2022-01-05T16:54:23Z

Hey @rsimha! These files were changed:

build-system/compile/closure-compile.js
build-system/compile/compile.js

Hey @rcebulko! These files were changed:

build-system/tasks/check-types.js

samouri · 2022-01-05T16:58:08Z

@rcebulko: requesting a review of just the check-types.js file

rcebulko

🥳 🎉 🎂
This is awesome!
One thing to add to the description to-do list: Possibly clean up some babel transforms that existed to satisfy something Closure needed, such as #37233

rsimha

Yay!

rsimha · 2022-01-05T17:23:06Z

build-system/tasks/helpers.js

-  if (minify) {
-    log('Minifying multi-pass JS with', cyan('closure-compiler') + '...');
-  } else {
-    log('Compiling JS with', cyan('esbuild'), 'and', cyan('babel') + '...');
-  }


I think the distinction between minified and non-minified ("compiled", for lack of a better word) ought to remain in the logs. Perhaps change the first word of the sentence to minify ? 'Minifying' : 'Compiling'?

Is there a distinction between the two with esbuild?

Sure. I'll put that distinction back in via Compiling minified vs. Compiling unminified, which I think reads clearer.
WDYT of removing the details around esbuild/babel? I think those might be too much detail for this log (why are we excluding the other dozen tools...where do we draw the line..)

Sure. I'll put that distinction back in via Compiling minified vs. Compiling unminified, which I think reads clearer.

SGTM. 👍

WDYT of removing the details around esbuild/babel? I think those might be too much detail for this log (why are we excluding the other dozen tools...where do we draw the line..)

Fair point. The logging was originally added back when there was a substantial difference between minified (only closure) and unminified (only babel). It's less accurate / useful today given all the recent tooling changes.

rcebulko · 2022-01-05T17:32:46Z

build-system/tasks/check-types.js

@@ -6,7 +6,6 @@ const {
 const {
  displayLifecycleDebugging,


Does displayLifecycleDebugging do anything here?

rcebulko · 2022-01-05T17:34:34Z

build-system/tasks/check-types.js

  // Prepare build environment
  process.env.NODE_ENV = 'production';
-  cleanupBuildDir();
  typecheckNewServer();


AFAICT the debug flag no longer has meaning in this file

Should I remove? I don't know what debug/displayLifecycleDebugging are for.

A quick inspection suggestions displayLifecycleDebugging should only matter if the debug method in that same file is called, and I don't see where it would be called here, but it's possible somewhere in the dependency chain it does get called. Not certain without testing it out

displayLifecycleDebugging was added in #27700 as a way to print the output at various stages of compilation: babel, closure, more babel, etc. It's triggered by the --debug flag, and was previously used by dist and check-types (both of which used closure).

It's probably far less useful without closure and the pre- and post- babel steps. Okay to remove IMO. Might be worth checking in with someone like @jridgewell to make sure.

…nce-attr-to-hero-img * 'main' of github.com:ampproject/amphtml: (525 commits) mathml storybook: supply missing component definition. (#37326) storybook: Iframe --> BentoIframe (#37327) 🖍 [Story system layer] New ad badge (#37311) 🐛 [amp story] Replay/next page button bug fix (#37316) 🚀 [Story performance] Remove affiliate links (#37280) Compiler: Add amp-carousel-0.1 to the builder map (#37308) ⏪ [Story system layer] Audio icon disappears when story has background audio (#37314) 🚀 [Story performance] Remove story access (#37281) Fix remapping esbuild output on Windows (#37312) 🐛 adds in correct weight for amp-story-product-tag text (#37188) typechecking carousel: remove shame files (#37213) Use remapping to remap minified sourcemap into source code (#37238) SwG Release 0.1.22.199 (#37310) 🐛 Adds microsoft-edge protocol (#34168) Sync for validator cpp engine and cpp htmlparser (#37292) ✨ amp-story-shopping Updated currency with product price and correct Localized currency (#37249) ✨[Smartadserver ad extension] Implement support for Fast Fetch (#36991) Remove client-side-experiments-config.json from this repo (#37304) 🚮 Remove closure compiler logic from build-system. (#37296) 🌐 Added RTL ordering i18n for amp story shopping tag (#37252) ...

samouri changed the title ~~🚮 Remove most traces of closure from build-system.~~ 🚮 Remove closure compiler logic from build-system. Jan 4, 2022

samouri force-pushed the rm-closure branch from c0efb60 to 25bf2a9 Compare January 4, 2022 22:24

Remove most traces of closure from build-system.

c6d0420

samouri force-pushed the rm-closure branch from 25bf2a9 to c6d0420 Compare January 5, 2022 03:52

samouri self-assigned this Jan 5, 2022

remove sources.js, restore closure to deps for validator.

634e158

samouri requested a review from rsimha January 5, 2022 16:54

samouri marked this pull request as ready for review January 5, 2022 16:54

samouri requested a review from jridgewell January 5, 2022 16:54

rcebulko approved these changes Jan 5, 2022

View reviewed changes

rsimha approved these changes Jan 5, 2022

View reviewed changes

rcebulko reviewed Jan 5, 2022

View reviewed changes

review feedback

716a359

samouri merged commit 6bfa3ac into ampproject:main Jan 5, 2022

samouri deleted the rm-closure branch January 5, 2022 21:04

ampprojectbot added the PR use: In Beta / Experimental label Jan 12, 2022

samouri mentioned this pull request Jan 18, 2022

wg-performance Status Update 2022-01-17 ampproject/wg-performance#103

Closed

ampprojectbot added the PR use: In Stable label Jan 18, 2022

Conversation

samouri commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amp-owners-bot bot commented Jan 5, 2022

Uh oh!

samouri commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rcebulko left a comment

Choose a reason for hiding this comment

Uh oh!

rsimha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

samouri commented Jan 4, 2022 •

edited

Loading

samouri commented Jan 5, 2022 •

edited

Loading