Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add <template> support for bento-date-display and bento-date-countdown components #37146

Closed
wants to merge 17 commits into from

Conversation

dmanek
Copy link
Contributor

@dmanek dmanek commented Dec 8, 2021

Support <template> as an alternative to mustache templates to render user customized bento-date-display & bento-date-countdown.

Fixes #36619

@dmanek dmanek changed the title initial implementation to support <template> Add <template> support for bento-date-display and bento-date-countdown components Jan 11, 2022
@dmanek dmanek marked this pull request as ready for review January 12, 2022 02:04
*/
export function getTemplateElement(element) {
return element.hasAttribute('template')
? element.ownerDocument.getElementById(element.getAttribute('template'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we verify that this is actually a <template>? Eg, doing a querySelector(`template#${getAttribute('template')}`)

let templateStr = '';
// `template.content` returns a Document Fragment, so iterate over it's
// children to return as a string.
for (let i = 0; i < template.content.children.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Store children as a local to prevent needless JS -> C++ boundary transitions.

const variables = getVariablesAsString(data);
const templateContents = getTemplateContentsAsString(template);
return new Function(
`${variables} return \`${escapeBackticks(templateContents)}\`;`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially eval, which means it can never work when served via Cache (or on any publisher with a decent CSP). This needs to be discussed in design review, because we can't allow this current solution.

<h1>Date Countdown</h1>
<bento-date-countdown id="one" timeleft-ms="0">
<template>
<p>{"d":"${d}","dd":"${dd}","h":"${h}","hh":"${hh}","m":"${m}","mm":"${mm}","s":"${s}","ss":"${ss}"}</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason you had to wrap this in <p> tags is because you used children instead of childNodes in the template helpers.

import {BentoDateCountdown} from './component';

export class BaseElement extends PreactBaseElement {}
export class BaseElement extends PreactBaseElement {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to sanitize the result on the AMP layer. In the meantime, it's okay if you disable the feature on AMP to submit this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strategy for rendering from user-provided templates on Bento standalone
3 participants