Static HTML Template helper by jridgewell · Pull Request #14623 · ampproject/amphtml

jridgewell · 2018-04-13T23:32:39Z

Create static HTML DOM trees using a HTML like syntax. Leading/trailing text (including whitespace) is dropped, and only a single root element in the template is supported.

This is only available for static trees. Dynamic variables in the string are not permitted, and will result in linting errors and runtime errors.

Valid uses are:

// Yes, the variable name must be "html".
const html = htmlFor(doc || el);

const div = html`<div><p>Hello World</p></div>`;

Or, you can use it directly as the template literal tag:

const div = htmlFor(doc || el)`<div><p>Hello World</p></div>`;

Closes #14529, jump starts #13906.
/cc @alanorozco, @kristoferbaxter

kristoferbaxter

Left a few questions, I don't understand why known arguments at tagged template literal creation time cannot be included.

Other than that, LGTM

kristoferbaxter · 2018-04-16T15:32:20Z

src/service/viewport/viewport-binding-ios-embed-wrapper.js

-    this.win.document.documentElement.classList.add('i-amphtml-ios-embed');
+    const documentElement = this.win.document.documentElement;
+    const topClasses = documentElement.className;
+    documentElement.className = 'i-amphtml-ios-embed';


Nice clean up.

I'm curious why className was set to an empty value and the classList was added to afterward in the previous code. Is there a subtle behaviour specific to iOS?

Looks like an oversight from https://github.com/ampproject/amphtml/pull/6276/files#diff-6eedd5ca45df1b5aee7d616b017d360aR1419.

kristoferbaxter · 2018-04-16T15:42:27Z

src/layout.js

+        <img class="i-amphtml-intrinsic-sizer" />
+      </i-amphtml-sizer>`;
+    const intrinsicSizer = sizer.firstElementChild;
    intrinsicSizer.setAttribute('src',


Perhaps I am missing something. Why can't this attribute be included in the template literal?

The src requires dynamic height and width, which aren't statically analyzable.

kristoferbaxter · 2018-04-16T15:43:27Z

src/preconnect.js

-    preload.setAttribute('rel', 'preload');
+    const preload = htmlFor(this.document_)`
+        <link rel="preload" referrerpolicy="origin" />`;
    preload.setAttribute('href', url);


Same question here, is there a reason the url cannot be included here in the tagged template literal?

The url is not statically analyzable, so there's not guarantee that it's not malicious.

alanorozco · 2018-04-16T19:58:28Z

/cc @newmuis @Enriqe @gmajoulet for amp-story.

IMO this should be used instead of SimpleTemplate where the templates are static.

Dynamic templating still unsolved.

newmuis · 2018-04-16T20:30:02Z

@alanorozco Thanks to our naive localization approach, pretty much none of our templates are static anymore (translated strings get swapped in at runtime) ☹️

jridgewell · 2018-04-16T22:32:31Z

(translated strings get swapped in at runtime)

Using the html helper is still a better approach here:

diff --git a/extensions/amp-story/0.1/amp-story.js b/extensions/amp-story/0.1/amp-story.js
--- a/extensions/amp-story/0.1/amp-story.js
+++ b/extensions/amp-story/0.1/amp-story.js
  * Container for "pill-style" share widget, rendered on desktop.
  * @private @const {!./simple-template.ElementDef}
  */
-const SHARE_WIDGET_PILL_CONTAINER = {
-  tag: 'div',
-  attrs: dict({'class': 'i-amphtml-story-share-pill'}),
-  children: [
-    {
-      tag: 'span',
-      attrs: dict({'class': 'i-amphtml-story-share-pill-label'}),
-      localizedStringId:
-          LocalizedStringId.AMP_STORY_SYSTEM_LAYER_SHARE_WIDGET_LABEL,
-    },
-  ],
-};
+const shareWidgetPillContainer = html`
+    <div class="i-amphtml-story-share-pill">
+      <span class="i-amphtml-story-share-pill-label"></span>
+    </div>`
+shareWidgetPillContainer.querySelector('i-amphtml-story-share-pill-label')
+    .innerText = LocalizedStringId.AMP_STORY_SYSTEM_LAYER_SHARE_WIDGET_LABEL;

jridgewell · 2018-04-17T00:13:27Z

Size	Build	Dist
Pre:	334.31KB	76.6KB
Post:	335.54KB	76.84KB
After #14657 and #14658		76.72KB

googlebot · 2018-04-17T19:35:18Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-04-17T19:58:51Z

CLAs look good, thanks!

Create static HTML DOM trees using a HTML like syntax. This is **only** available for static trees. **Dynamic variables in the string are not permitted**.

* Static HTML Template helper Create static HTML DOM trees using a HTML like syntax. This is **only** available for static trees. **Dynamic variables in the string are not permitted**. * Typecast * Update fixed position tests * Do not create HTML elements using html helper * Insensitive lint * lint * Add htmlRefs function * Bump max filesize * Tests * Revert changes to eslint-plugin-amphtml-internal version * Lint

jridgewell requested review from alanorozco and kristoferbaxter April 13, 2018 23:32

googlebot added the cla: yes label Apr 13, 2018

kristoferbaxter approved these changes Apr 16, 2018

View reviewed changes

alanorozco approved these changes Apr 16, 2018

View reviewed changes

jridgewell mentioned this pull request Apr 16, 2018

Build step: Reduce static template whitespace #14657

Closed

jridgewell force-pushed the static-template branch from b509aed to 5ddba2e Compare April 16, 2018 23:55

jridgewell mentioned this pull request Apr 17, 2018

Build Step: Transform Tagged Template Literals into Simple Code #14658

Closed

newmuis mentioned this pull request Apr 17, 2018

Migrate amp-story to use static HTML template helper #14659

Closed

jridgewell force-pushed the static-template branch from 5ddba2e to c85d4af Compare April 17, 2018 02:51

googlebot added cla: no and removed cla: yes labels Apr 17, 2018

jridgewell force-pushed the static-template branch from 244d511 to 3c606ba Compare April 17, 2018 19:58

googlebot added cla: yes and removed cla: no labels Apr 17, 2018

jridgewell force-pushed the static-template branch from 0fef4f7 to 14bd80d Compare April 17, 2018 22:34

jridgewell and others added 8 commits April 17, 2018 19:34

Static HTML Template helper

cb4558d

Create static HTML DOM trees using a HTML like syntax. This is **only** available for static trees. **Dynamic variables in the string are not permitted**.

Typecast

d4c6554

Update fixed position tests

d1e2b81

Do not create HTML elements using html helper

3a2303f

Insensitive lint

4dc1208

lint

89a947e

Add htmlRefs function

148f9ce

Bump max filesize

0effd8f

jridgewell added 2 commits April 17, 2018 19:34

Tests

c4edd87

Revert changes to eslint-plugin-amphtml-internal version

ed7c01e

jridgewell force-pushed the static-template branch from 14bd80d to ed7c01e Compare April 17, 2018 23:35

Lint

1b3bb7c

jridgewell merged commit 333a47c into ampproject:master Apr 18, 2018

jridgewell deleted the static-template branch April 18, 2018 00:57

rsimha mentioned this pull request May 4, 2018

Leaner AMP: Stop the bleeding (feeding?) #14394

Closed

alanorozco mentioned this pull request May 8, 2018

Build step: Collapse static template comments #15145

Closed

alanorozco mentioned this pull request May 30, 2018

AMP components lack a template rendering solution #13906

Closed

newmuis mentioned this pull request Jun 5, 2018

Proposal: Use mustache as template language in amp-story #13992

Closed

snyk-bot mentioned this pull request Jul 12, 2021

[Snyk] Upgrade eslint from 7.14.0 to 7.29.0 Zintexiss/amphtml#28

Closed

Conversation

jridgewell commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristoferbaxter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristoferbaxter Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

jridgewell Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

kristoferbaxter Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

jridgewell Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

kristoferbaxter Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

jridgewell Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

alanorozco commented Apr 16, 2018

Uh oh!

newmuis commented Apr 16, 2018

Uh oh!

jridgewell commented Apr 16, 2018

Uh oh!

jridgewell commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Apr 17, 2018

Uh oh!

googlebot commented Apr 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jridgewell commented Apr 13, 2018 •

edited

Loading

kristoferbaxter left a comment •

edited

Loading

jridgewell commented Apr 17, 2018 •

edited

Loading