Skip to content

npm builds should not use options.wrapper#35766

Merged
caroqliu merged 2 commits intoampproject:mainfrom
caroqliu:bento-npm-builds
Aug 23, 2021
Merged

npm builds should not use options.wrapper#35766
caroqliu merged 2 commits intoampproject:mainfrom
caroqliu:bento-npm-builds

Conversation

@caroqliu
Copy link
Copy Markdown
Contributor

@caroqliu caroqliu commented Aug 23, 2021 •
edited
Loading

The wrapper for bento components is defined for use with custom element builds, not preact/react builds. For npm binaries, the "wrapper": "bento" designation was being split up via this helper and written as a bent to the start of all npm build binaries:

amphtml/build-system/tasks/helpers.js

Lines 426 to 439 in c43ba12

/**
* Splits up the wrapper to compute the banner and footer
* @return {Object}
*/
function splitWrapper() {
const wrapper = options.wrapper || wrappers.none;
const sentinel = '<%= contents %>';
const start = wrapper.indexOf(sentinel);
return {
banner: {js: wrapper.slice(0, start)},
footer: {js: wrapper.slice(start + sentinel.length)},
};
}
const {banner, footer} = splitWrapper();

Another option could be to guard for an invalid wrapper, which also fixes the issue (but may have more consequences given more code paths may be affected):

diff --git a/build-system/tasks/helpers.js b/build-system/tasks/helpers.js
index a92748b0ee..3cc029ab2a 100644
--- a/build-system/tasks/helpers.js
+++ b/build-system/tasks/helpers.js
@@ -428,9 +428,13 @@ async function esbuildCompile(srcDir, srcFilename, destDir, options) {
    * @return {Object}
    */
   function splitWrapper() {
-    const wrapper = options.wrapper || wrappers.none;
     const sentinel = '<%= contents %>';
-    const start = wrapper.indexOf(sentinel);
+    let wrapper = options.wrapper || wrappers.none;
+    let start = wrapper.indexOf(sentinel);
+    if (start === -1) {
+      wrapper = wrappers.none;
+      start = wrapper.indexOf(sentinel);
+    }
     return {
       banner: {js: wrapper.slice(0, start)},
       footer: {js: wrapper.slice(start + sentinel.length)},

Fixes #35741

@caroqliu caroqliu requested a review from estherkim August 23, 2021 15:17
@caroqliu caroqliu merged commit 68b7333 into ampproject:main Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estherkim estherkim estherkim approved these changes

Assignees

No one assigned

Labels

PR use: In Beta / Experimental PR use: In LTS PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bento: Errant bent statement breaks component

3 participants

@caroqliu @estherkim @ampprojectbot