build-system: improve terser/esbuild integration#35466
build-system: improve terser/esbuild integration#35466samouri merged 4 commits intoampproject:mainfrom
Conversation
948deb2 to
2996908
Compare
| * @return {!Promise} | ||
| * @param {string} code | ||
| * @param {string} map | ||
| * @return {!Promise<{code: string, map: *, error?: Error}>} |
There was a problem hiding this comment.
self-nit: error? should be removed
rsimha
left a comment
There was a problem hiding this comment.
LGTM with some non-blocking questions / comments. Thank you!
| fs.outputFile(`${destFile}.map`, map), | ||
| ]); | ||
|
|
||
| // TODO: finishBundle should operate in-mem instead of reading/writing from FS. |
There was a problem hiding this comment.
Agree with the sentiment behind this statement. In practice, the in-memory stuff we used to do with gulp streams once upon a time was way slower than the current direct-to-file-system operations. Going back to in-memory (without gulp) will be worth it if we can achieve yet another order of magnitude speed up. (Otherwise, may not be worth it as you point out.)
There was a problem hiding this comment.
Thank you for the context! I know we'll need to leave some high level funcs that operate on FS around for test infra, but I can't imagine in-mem being slower for this flow
build-system/tasks/helpers.js
Outdated
| fs.writeFileSync(`${filename}.map`, remapped.toString()); | ||
|
|
||
| const minified = await terser.minify(code, terserOptions); | ||
| return {code: minified.code ?? '', map: minified.map}; |
There was a problem hiding this comment.
I assume the nullish coalescing is to satisfy type checking? Could minified.code ever realistically be nullish, and if so, are we swallowing an error by defaulting to ''?
There was a problem hiding this comment.
Exactly right. I don't think it could be nullish. If you prefer, I could check for nullish higher up and throw in that case? I'm not sure what to do when the typing is nonsensical...maybe cast?
That said, maybe I"m being arrogant and there actually is a way for code to be nullish..
There was a problem hiding this comment.
Hmm. I only asked the question to make sure we didn't miss an easy trick. I don't actually have any ideas to make this better than it is. My guess is that any meaningful error during terser.minify will be thrown, and we'll never actually silently return an empty string. And even if we do, the very next step in our build pipeline will likely error out with a different meaningful error.
TL;DR: Okay to leave the code as is, given that this discussion now exists for posterity 😃
…tok-validation * 'main' of github.com:ampproject/amphtml: (72 commits) build: run amp lint --fix to address import order of jixie (ampproject#35513) ✨ [amp-analytics] Add Custom Browser Event Tracker (ampproject#35193) babel: teach amp mode transformer about #core/mode (ampproject#35477) 🚮 Remove experiment `amp-consent-granular-consent` (ampproject#35508) ♻️ Enable auto-sorting+grouping within src/ and 3p/ (ampproject#35454) 🐛 [amp-render] fix root-element stripping from amp-render with amp-bind (ampproject#35449) ✅ [Story interactive] Add Example Story for Detailed Results Component (ampproject#35450) 🐛 Fix error on bento example (ampproject#35490) 🐛 amp-story-grid-layer: Fix AMP invalidation error in documentation (ampproject#35503) 🐛 Fix code formatting (ampproject#35499) ✅ buildDom: add tests for amp-fit-text and amp-layout (ampproject#35494) ♻️ Remove unused imports (ampproject#35435) ✨ amp-connatix-player: iframe domain based on a property (ampproject#35179) Updated document with use cases of remote config (ampproject#35496) AMP.goBack: update documentation (ampproject#29290) 🏗 Allow the bundle-size job to run even if the builds were skipped (ampproject#35492) build-system: improve terser/esbuild integration (ampproject#35466) 🧪 [Story performance] Disable animations on first page to 50% (ampproject#35476) 📖 [Amp story] [Page attachments] Amp.dev Docs for New Page Attachment Features ampproject#34883 (ampproject#35338) 🚀 [Story interactive] Rewrite Image URL to Cached URL for Image Quizzes and Polls (ampproject#35375) ...
summary
The first in a series of small cleanups to our build helpers. The improvements are extractions from a much larger #35201
This PR
esbuildto write only to memory instead offsargv.esmremapping;, like we do with closure.Next up improvements:
compileWithEsbuildandcompileUnminifiedJsfinishBundleshould also operate in-mem...this may require more refactoring than its worth though