🚀 Remove doc css and base css from ESM build #26889
Conversation
|
This pull request introduces 1 alert when merging 0f1ceb1 into 911661b - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Code LGTM in terms of accomplishing what you set out for (except for the one typo).
I'm missing some of the context for this PR though.
I'm approving so as to not slow you down, and with the explicit hope that other approvers understand the context.
For my own learning, how are two CSS files being included for esm builds / why do they need a different method than the nomodule builds?
These files are included in the output from a valid transformed document, inline in the document. |
* master: (41 commits) custom-element: Minor test improvements (ampproject#26923) amp-pixel: Minor test improvements (ampproject#26918) viewer: Minor test improvements (ampproject#26906) dom: Minor test improvements (ampproject#26913) amp-action: Support whitelist lookup in AmpDocShadow (ampproject#26684) ✨ Update amp-access-scroll (ampproject#26810) 🚀 Remove doc css and base css from ESM build (ampproject#26889) 📖 [amp-story-player] Initial docs (ampproject#26606) Amp consent restrict fullscreen prod flag (ampproject#26909) 📖 Clarify SXG duration minimum (ampproject#26890) Improve test vendor requests macros (ampproject#26828) 🚀 Move scroll left and top macros out of url-replacement-impl (ampproject#25594) Update consent string maximum size to 200 bytes (ampproject#26741) ✨[amp-story-player] Adds tap-to-next/previous story (ampproject#26865) update owners file with correct syntax (ampproject#26899) amp-sticky-ad: Fix unit test (ampproject#26855) Add performance metrics to README (ampproject#26891) 🐛 Bug fix: check links test (ampproject#26739) ✨Idealmedia uniq ad (ampproject#25838) 📦 Update dependency jsdom to v16.2.0 (ampproject#26591) ...
| // For ESM Builds, exclude ampdoc and ampshared css from inclusion. | ||
| // These styles are guaranteed to already be present on elgible documents. | ||
| if (options.esmPassCompilation) { | ||
| srcs.push('!build/ampdoc.css.js', '!build/ampshared.css.js'); | ||
| } |
There was a problem hiding this comment.
Sorry about the late review (I was on vacation last week). With gulp v4, I know that negative globstars need to come at the end for them to be considered, because globs are evaluated in order. I'm not sure if the same rule applies to negative globs that don't use a wildcard.
All negative globstars in this file appear here:
amphtml/build-system/compile/compile.js
Lines 256 to 266 in 3d3c31e
Something to consider, as this file grows.
The ESM runtime is only used on valid transformed documents, including those from an AMP Cache.
This PR removes the duplicated CSS from a valid transformed document from the JavaScript runtime payload.