Skip to content

Commit

Permalink
[labs/ssr] Emit lit-node marker before node to support raw text nodes. (
Browse files Browse the repository at this point in the history 
#3667)

* Emit lit-node marker before node to support raw text nodes.
Fixes #3663

* Fix copy/paste mistake.
  • Loading branch information
@kevinpschaaf
kevinpschaaf committed Feb 14, 2023
1 parent 499985d commit e00f6f5
Show file tree
Hide file tree
Showing 5 changed files with 115 additions and 56 deletions.
6 changes: 6 additions & 0 deletions .changeset/soft-turtles-protect.md
View file
@@ -0,0 +1,6 @@
---
'@lit-labs/ssr': patch
'lit-html': patch
---

Improved how nodes with attribute/property/event/element bindings are rendered in SSR, to avoid adding comments inside of "raw text elements" like `<textarea>`. Fixes #3663.
77 changes: 44 additions & 33 deletions packages/labs/ssr/src/lib/render-value.ts
View file
Expand Up @@ -222,14 +222,18 @@ type Op =
* ```
*
* - `text`
* - Emit run of static text: `<div><span>Hello</span><span`
* - Emit run of static text: `<div><span>Hello</span>`
* - `possible-node-marker`
* - Emit `<!--lit-node n-->` marker since there are attribute parts
* - `text`
* - Emit run of static text: `<span`
* - `attribute-part`
* - Emit an AttributePart's value, e.g. ` class="bold"`
* - `text`
* - Emit run of static text: `>`
* - `child-part`
* - Emit the ChildPart's value, in this case a TemplateResult, thus we recurse
* into that template's opcodes
* - Emit the ChildPart's value, in this case a TemplateResult, thus we
* recurse into that template's opcodes
* - `text`
* - Emit run of static text: `/span></div>`
*
Expand All @@ -239,6 +243,9 @@ type Op =
* html`<x-foo staticAttr dynamicAttr=${value}><div>child</div>...</x-foo>`
* ```
*
* - `possible-node-marker`
* - Emit `<!--lit-node n-->` marker since there are attribute parts and we
* may emit the `defer-hydration` attribute on the node that follows
* - `text`
* - Emit open tag `<x-foo`
* - `custom-element-open`
Expand All @@ -254,9 +261,6 @@ type Op =
* - Emit `renderer.renderAttributes()`
* - `text`
* - Emit end of of open tag `>`
* - `possible-node-marker`
* - Emit `<!--lit-node n-->` marker if there were attribute parts or
* we needed to emit the `defer-hydration` attribute
* - `custom-element-shadow`
* - Emit `renderer.renderShadow()` (emits `<template shadowroot>` +
* recurses to emit `render()`)
Expand Down Expand Up @@ -362,22 +366,15 @@ const getTemplateOpcodes = (result: TemplateResult) => {
}
nodeIndex++;
} else if (isElementNode(node)) {
// Whether to flush the start tag. This is necessary if we're changing
// any of the attributes in the tag, so it's true for custom-elements
// which might reflect their own state, or any element with a binding.
let writeTag = false;
let boundAttributesCount = 0;

const tagName = node.tagName;
let ctor;

if (tagName.indexOf('-') !== -1) {
// Looking up the constructor here means that custom elements must be
// registered before rendering the first template that contains them.
ctor = customElements.get(tagName);
const ctor = customElements.get(tagName);
if (ctor !== undefined) {
// Write the start tag
writeTag = true;
// Mark that this is a custom element
node.isDefinedCustomElement = true;
ops.push({
Expand All @@ -393,12 +390,35 @@ const getTemplateOpcodes = (result: TemplateResult) => {
}
}
if (node.attrs.length > 0) {
const attrInfo = [] as Array<
[boolean, boolean, typeof node.attrs[0]]
>;
for (const attr of node.attrs) {
const isAttrBinding = attr.name.endsWith(boundAttributeSuffix);
const isElementBinding = attr.name.startsWith(marker);
if (isAttrBinding || isElementBinding) {
writeTag = true;
boundAttributesCount += 1;
}
attrInfo.push([isAttrBinding, isElementBinding, attr]);
}
if (boundAttributesCount > 0 || node.isDefinedCustomElement) {
// We (may) need to emit a `<!-- lit-node -->` comment marker to
// indicate the following node needs to be identified during
// hydration when it has bindings or if it is a custom element (and
// thus may need its `defer-hydration` to be removed, depending on
// the `deferHydration` setting). The marker is emitted as a
// previous sibling before the node in question, to avoid issues
// with void elements (which do not have children) and raw text
// elements (whose children are intepreted as text).
flushTo(node.sourceCodeLocation!.startTag!.startOffset);
ops.push({
type: 'possible-node-marker',
boundAttributesCount,
nodeIndex,
});
}
for (const [isAttrBinding, isElementBinding, attr] of attrInfo) {
if (isAttrBinding || isElementBinding) {
// Note that although we emit a lit-node comment marker for any
// nodes with bindings, we don't account for it in the nodeIndex because
// that will not be injected into the client template
Expand Down Expand Up @@ -430,7 +450,7 @@ const getTemplateOpcodes = (result: TemplateResult) => {
: AttributePart,
strings,
tagName: tagName.toUpperCase(),
useCustomElementInstance: ctor !== undefined,
useCustomElementInstance: node.isDefinedCustomElement,
});
} else {
ops.push({
Expand All @@ -453,25 +473,16 @@ const getTemplateOpcodes = (result: TemplateResult) => {
}
}

if (writeTag) {
if (node.isDefinedCustomElement) {
flushTo(node.sourceCodeLocation!.startTag!.endOffset - 1);
ops.push({
type: 'custom-element-attributes',
});
flush('>');
skipTo(node.sourceCodeLocation!.startTag!.endOffset);
} else {
flushTo(node.sourceCodeLocation!.startTag!.endOffset);
}
if (node.isDefinedCustomElement) {
// For custom elements, add an opcode to write out attributes,
// close the tag, and then add an opcode to write the shadow
// root
flushTo(node.sourceCodeLocation!.startTag!.endOffset - 1);
ops.push({
type: 'possible-node-marker',
boundAttributesCount,
nodeIndex,
type: 'custom-element-attributes',
});
}

if (ctor !== undefined) {
flush('>');
skipTo(node.sourceCodeLocation!.startTag!.endOffset);
ops.push({
type: 'custom-element-shadow',
});
Expand Down
43 changes: 43 additions & 0 deletions packages/labs/ssr/src/test/integration/tests/basic.ts
View file
Expand Up @@ -1628,6 +1628,49 @@ export const tests: {[name: string]: SSRTest} = {
stableSelectors: ['input'],
},

'AttributePart on raw text element in shadow root': {
// Regression test for https://github.com/lit/lit/issues/3663.
//
// Confirms that attribute bindings to raw text elements now
// work as expected.
registerElements() {
class RawElementHost extends LitElement {
@property()
text = 'hello';

override render() {
return html`<textarea .value=${this.text}></textarea>`;
}
}
customElements.define('raw-element-host', RawElementHost);
},
render() {
return html`<raw-element-host></raw-element-host>`;
},
expectations: [
{
args: [],
html: '<raw-element-host></raw-element-host>',
async check(assert: Chai.Assert, dom: HTMLElement) {
const host = dom.querySelector('raw-element-host') as LitElement & {
text: string;
};
assert.instanceOf(host, LitElement);
assert.equal(host.text, 'hello');

await host.updateComplete;
const textarea = host.shadowRoot?.querySelector('textarea');
assert.equal(textarea?.value, 'hello');

host.text = 'goodbye';
await host.updateComplete;
assert.equal(textarea?.value, 'goodbye');
},
},
],
stableSelectors: ['textarea'],
},

/******************************************************
* PropertyPart tests
******************************************************/
Expand Down
36 changes: 18 additions & 18 deletions packages/labs/ssr/src/test/lib/render-lit_test.ts
View file
Expand Up @@ -118,7 +118,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
const result = await render(templateWithAttributeExpression('foo'));
assert.is(
result,
`<!--lit-part FAR9hgjJqTI=--><div class="foo"><!--lit-node 0--></div><!--/lit-part-->`
`<!--lit-part FAR9hgjJqTI=--><!--lit-node 0--><div class="foo"></div><!--/lit-part-->`
);
});

Expand All @@ -127,7 +127,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
const result = await render(inputTemplateWithAttributeExpression('foo'));
assert.is(
result,
`<!--lit-part AYwG7rAvcnw=--><input x="foo"><!--lit-node 0--><!--/lit-part-->`
`<!--lit-part AYwG7rAvcnw=--><!--lit-node 0--><input x="foo"><!--/lit-part-->`
);
});

Expand All @@ -144,7 +144,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
);
assert.is(
result,
`<!--lit-part BIugdiAuV4I=--><input x="foo"><!--lit-node 0--><p>hi</p></input><!--/lit-part-->`
`<!--lit-part BIugdiAuV4I=--><!--lit-node 0--><input x="foo"><p>hi</p></input><!--/lit-part-->`
);
});

Expand All @@ -156,7 +156,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
// Has marker attribute for number of bound attributes.
assert.is(
result,
`<!--lit-part FQlA2/EioQk=--><div x="foo" y="bar" z="not-dynamic"><!--lit-node 0--></div><!--/lit-part-->`
`<!--lit-part FQlA2/EioQk=--><!--lit-node 0--><div x="foo" y="bar" z="not-dynamic"></div><!--/lit-part-->`
);
});

Expand All @@ -167,7 +167,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
);
assert.is(
result,
`<!--lit-part D+PQMst9obo=--><div test="a foo b bar c"><!--lit-node 0--></div><!--/lit-part-->`
`<!--lit-part D+PQMst9obo=--><!--lit-node 0--><div test="a foo b bar c"></div><!--/lit-part-->`
);
});

Expand All @@ -178,7 +178,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
const result = await render(inputTemplateWithValueProperty('foo'));
assert.is(
result,
`<!--lit-part AxWziS+Adpk=--><input value="foo"><!--lit-node 0--><!--/lit-part-->`
`<!--lit-part AxWziS+Adpk=--><!--lit-node 0--><input value="foo"><!--/lit-part-->`
);
});

Expand All @@ -187,7 +187,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
const result = await render(elementTemplateWithClassNameProperty('foo'));
assert.is(
result,
`<!--lit-part I7NxrdZ/Zxo=--><div class="foo"><!--lit-node 0--></div><!--/lit-part-->`
`<!--lit-part I7NxrdZ/Zxo=--><!--lit-node 0--><div class="foo"></div><!--/lit-part-->`
);
});

Expand All @@ -196,7 +196,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
const result = await render(elementTemplateWithClassnameProperty('foo'));
assert.is(
result,
`<!--lit-part I7NxrbZzZGA=--><div ><!--lit-node 0--></div><!--/lit-part-->`
`<!--lit-part I7NxrbZzZGA=--><!--lit-node 0--><div ></div><!--/lit-part-->`
);
});

Expand All @@ -205,7 +205,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
const result = await render(elementTemplateWithIDProperty('foo'));
assert.is(
result,
`<!--lit-part IgnmhhM3LsA=--><div id="foo"><!--lit-node 0--></div><!--/lit-part-->`
`<!--lit-part IgnmhhM3LsA=--><!--lit-node 0--><div id="foo"></div><!--/lit-part-->`
);
});

Expand Down Expand Up @@ -276,7 +276,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
// TODO: we'd like to remove the extra space in the start tag
assert.is(
result,
`<!--lit-part v2CxGIW+qHI=--><test-property ><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->bar<!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
`<!--lit-part v2CxGIW+qHI=--><!--lit-node 0--><test-property ><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->bar<!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
);
});

Expand All @@ -286,7 +286,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
// TODO: we'd like to remove the extra space in the start tag
assert.is(
result,
`<!--lit-part ZI1U/5CYP1o=--><test-property foo="bar"><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->bar<!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
`<!--lit-part ZI1U/5CYP1o=--><!--lit-node 0--><test-property foo="bar"><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->bar<!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
);
});

Expand All @@ -296,7 +296,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
// TODO: we'd like to remove the extra space in the start tag
assert.is(
result,
`<!--lit-part ZI1U/5CYP1o=--><test-property foo><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
`<!--lit-part ZI1U/5CYP1o=--><!--lit-node 0--><test-property foo><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
);
});

Expand All @@ -306,7 +306,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
// TODO: we'd like to remove the extra space in the start tag
assert.is(
result,
`<!--lit-part ZI1U/5CYP1o=--><test-property foo><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
`<!--lit-part ZI1U/5CYP1o=--><!--lit-node 0--><test-property foo><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
);
});

Expand All @@ -316,7 +316,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
// TODO: we'd like to remove the extra space in the start tag
assert.is(
result,
`<!--lit-part ZI1U/5CYP1o=--><test-property foo><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
`<!--lit-part ZI1U/5CYP1o=--><!--lit-node 0--><test-property foo><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
);
});

Expand All @@ -326,7 +326,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
// TODO: we'd like to remove the extra space in the start tag
assert.is(
result,
`<!--lit-part 7z41MJchKXM=--><test-reflected-properties bar baz="default reflected string" reflect-foo="badazzled"><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part--><!--/lit-part--></template></test-reflected-properties><!--/lit-part-->`
`<!--lit-part 7z41MJchKXM=--><!--lit-node 0--><test-reflected-properties bar baz="default reflected string" reflect-foo="badazzled"><template shadowroot="open" shadowrootmode="open"><!--lit-part--><!--/lit-part--></template></test-reflected-properties><!--/lit-part-->`
);
});

Expand All @@ -345,7 +345,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
// TODO: we'd like to remove the extra space in the start tag
assert.is(
result,
`<!--lit-part Q0bbGrx71ic=--><test-will-update ><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->Foo Bar<!--/lit-part--></main><!--/lit-part--></template></test-will-update><!--/lit-part-->`
`<!--lit-part Q0bbGrx71ic=--><!--lit-node 0--><test-will-update ><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->Foo Bar<!--/lit-part--></main><!--/lit-part--></template></test-will-update><!--/lit-part-->`
);
});

Expand Down Expand Up @@ -442,7 +442,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
const result = await render(classMapDirective);
assert.is(
result,
'<!--lit-part PkF/hiJU4II=--><div class=" a c "><!--lit-node 0--></div><!--/lit-part-->'
'<!--lit-part PkF/hiJU4II=--><!--lit-node 0--><div class=" a c "></div><!--/lit-part-->'
);
});

Expand All @@ -451,7 +451,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
const result = await render(classMapDirectiveMultiBinding);
assert.is(
result,
'<!--lit-part pNgepkKFbd0=--><div class="z hi a c"><!--lit-node 0--></div><!--/lit-part-->'
'<!--lit-part pNgepkKFbd0=--><!--lit-node 0--><div class="z hi a c"></div><!--/lit-part-->'
);
});

Expand Down
9 changes: 4 additions & 5 deletions packages/lit-html/src/experimental-hydrate.ts
View file
Expand Up @@ -348,11 +348,10 @@ const createAttributeParts = (
const match = /lit-node (\d+)/.exec(comment.data)!;
const nodeIndex = parseInt(match[1]);

// For void elements, the node the comment was referring to will be
// the previousSibling; for non-void elements, the comment is guaranteed
// to be the first child of the element (i.e. it won't have a previousSibling
// meaning it should use the parentElement)
const node = comment.previousElementSibling ?? comment.parentElement;
// Node markers are added as a previous sibling to identify elements
// with attribute/property/element/event bindings or custom elements
// whose `defer-hydration` attribute needs to be removed
const node = comment.nextElementSibling;
if (node === null) {
throw new Error('could not find node for attribute parts');
}
Expand Down

0 comments on commit e00f6f5

Please sign in to comment.