Skip to content

perf(template-compiler): optimize all static nodes #2969

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 2, 2022
Merged

perf(template-compiler): optimize all static nodes #2969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a8a5de
perf(template-compiler): optimize all static nodes
nolanlawson Jul 29, 2022
c14770f
fix: fix ie11
nolanlawson Aug 1, 2022
5d7fa16
fix: fix more elements in IE11
nolanlawson Aug 1, 2022
9294670
fix: switch to firstChild
nolanlawson Aug 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions packages/@lwc/engine-dom/src/createFragment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Copyright (c) 2018, salesforce.com, inc.
* All rights reserved.
* SPDX-License-Identifier: MIT
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/
import { isUndefined } from '@lwc/shared';

const SUPPORTS_TEMPLATE = typeof HTMLTemplateElement === 'function';

export let createFragment: (html: string) => Node | null;

if (SUPPORTS_TEMPLATE) {
// Parse the fragment HTML string into DOM
createFragment = function (html: string) {
const template = document.createElement('template');
template.innerHTML = html;
return template.content.firstChild;
};
Copy link
Contributor Author

@nolanlawson nolanlawson Aug 1, 2022

Choose a reason for hiding this comment

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

I considered caching and re-using the template here, but I decided against it because:

  1. This is just a one-time setup cost anyway – it wouldn't improve any of our benchmarks
  2. Holding onto the memory from the last HTML in the cached template strikes me as a little weird
  3. I'm not even sure if it's a big perf improvement in the first place

We might explore caching the template in a follow-up PR, though.

} else {
// In browsers that don't support <template> (e.g. IE11), we need to be careful to wrap elements like
// <td> in the proper container elements (e.g. <tbody>), because otherwise they will be parsed as null.

// Via https://github.com/webcomponents/polyfills/blob/ee1db33/packages/template/template.js#L273-L280
// With other elements added from:
// https://github.com/sindresorhus/html-tags/blob/95dcdd5/index.js
// Using the test:
// document.createRange().createContextualFragment(`<${tag}></${tag}>`).firstChild === null
// And omitting <html>, <head>, and <body> as these are not practical in an LWC component.
const topLevelWrappingMap: { [key: string]: string[] } = {
caption: ['table'],
col: ['colgroup', 'table'],
colgroup: ['table'],
option: ['select'],
Copy link
Contributor

@jodarove jodarove Aug 1, 2022

Choose a reason for hiding this comment

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

does tfoot, colgroup, tbody and caption also need to be on this map? (I don't think you can add head, body and html on an lwc template)

Copy link
Contributor Author

@nolanlawson nolanlawson Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nice catch! Fixed and added a test to make sure it actually works in IE11. I did not include head/body/html.

tbody: ['table'],
td: ['tr', 'tbody', 'table'],
th: ['tr', 'tbody', 'table'],
thead: ['table'],
tfoot: ['table'],
tr: ['tbody', 'table'],
};

// Via https://github.com/webcomponents/polyfills/blob/ee1db33/packages/template/template.js#L282-L288
const getTagName = function (text: string) {
return (/<([a-z][^/\0>\x20\t\r\n\f]+)/i.exec(text) || ['', ''])[1].toLowerCase();
};

// Via https://github.com/webcomponents/polyfills/blob/ee1db33/packages/template/template.js#L295-L320
createFragment = function (html: string) {
const wrapperTags = topLevelWrappingMap[getTagName(html)];
if (!isUndefined(wrapperTags)) {
for (const wrapperTag of wrapperTags) {
html = `<${wrapperTag}>${html}</${wrapperTag}>`;
}
}

// For IE11, the document title must not be undefined, but it can be an empty string
// https://developer.mozilla.org/en-US/docs/Web/API/DOMImplementation/createHTMLDocument#browser_compatibility
const doc = document.implementation.createHTMLDocument('');
doc.body.innerHTML = html;

let content: Node = doc.body;
if (!isUndefined(wrapperTags)) {
for (let i = 0; i < wrapperTags.length; i++) {
Copy link
Contributor

@jodarove jodarove Aug 1, 2022

Choose a reason for hiding this comment

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

Suggested change
for (let i = 0; i < wrapperTags.length; i++) {
for (let i = 0, n = wrapperTags.length; i < n; i++) {

Copy link
Contributor Author

@nolanlawson nolanlawson Aug 1, 2022

Choose a reason for hiding this comment

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

Last I checked, JS engines have optimized this; there's no need to cache the length.

There is some discussion on SO, although sadly all the JSPerf links are dead now. 😢

Copy link
Contributor Author

@nolanlawson nolanlawson Aug 1, 2022

Choose a reason for hiding this comment

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

OK, found a post from a V8 engineer; we do not need to cache length. 😁

content = content.firstChild!;
}
}

return content.firstChild;
};
}
6 changes: 1 addition & 5 deletions packages/@lwc/engine-dom/src/renderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
StringToLowerCase,
} from '@lwc/shared';
import { insertStylesheet } from './styles';

import { createFragment } from './createFragment';
let getCustomElement: any;
let defineCustomElement: any;
let HTMLElementConstructor;
Expand Down Expand Up @@ -104,10 +104,6 @@ function cloneNode(node: Node, deep: boolean): Node {
return node.cloneNode(deep);
}

function createFragment(html: string): Node | null {
return document.createRange().createContextualFragment(html).firstChild;
}

function createElement(tagName: string, namespace?: string): Element {
return isUndefined(namespace)
? document.createElement(tagName)
Expand Down
44 changes: 43 additions & 1 deletion packages/@lwc/integration-karma/test/static-content/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import Table from 'x/table';
import SvgPath from 'x/svgPath';
import SvgPathInDiv from 'x/svgPathInDiv';
import SvgPathInG from 'x/svgPathInG';
import StaticUnsafeTopLevel from 'x/staticUnsafeTopLevel';

// In compat mode, the component will always render in synthetic mode with the scope attribute
if (!process.env.NATIVE_SHADOW && !process.env.COMPAT) {
Expand Down Expand Up @@ -169,7 +170,7 @@ describe('svg and static content', () => {
});
});

describe('tables and static content', () => {
describe('elements that cannot be parsed as top-level', () => {
it('should work with a static <td>', () => {
const elm = createElement('x-table', { is: Table });
document.body.appendChild(elm);
Expand All @@ -183,6 +184,47 @@ describe('tables and static content', () => {
expect(elm.shadowRoot.querySelector('td').textContent).toEqual('');
});
});

it('works for all elements that cannot be safely parsed as top-level', () => {
const elm = createElement('x-static-unsafe-top-level', { is: StaticUnsafeTopLevel });
document.body.appendChild(elm);

const getChildrenTagNames = () => {
const result = [];
const { children } = elm.shadowRoot;
for (let i = 0; i < children.length; i++) {
result.push(children[i].tagName.toLowerCase());
}
return result;
};

const expectedChildren = [
'caption',
'col',
'colgroup',
'tbody',
'td',
'tfoot',
'th',
'thead',
'tr',
];

expect(getChildrenTagNames()).toEqual([]);
elm.doRender = true;
return Promise.resolve()
.then(() => {
expect(getChildrenTagNames()).toEqual(expectedChildren);
elm.doRender = false;
})
.then(() => {
expect(getChildrenTagNames()).toEqual([]);
elm.doRender = true;
})
.then(() => {
expect(getChildrenTagNames()).toEqual(expectedChildren);
});
});
});

describe('template literal escaping', () => {
Expand Down
31 changes: 31 additions & 0 deletions ...wc/integration-karma/test/static-content/x/staticUnsafeTopLevel/staticUnsafeTopLevel.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<template>
<!-- Separating these with individual "if" blocks causes the compiler to make each one a static fragment -->
<template if:true={doRender}>
<caption></caption>
</template>
<template if:true={doRender}>
<col>
</template>
<template if:true={doRender}>
<colgroup></colgroup>
</template>
<template if:true={doRender}>
<tbody></tbody>
</template>
<template if:true={doRender}>
<td></td>
</template>
<template if:true={doRender}>
<tfoot></tfoot>
</template>
<template if:true={doRender}>
<th></th>
</template>
<template if:true={doRender}>
<thead></thead>
</template>
<template if:true={doRender}>
<tr></tr>
</template>
</template>
</template>
5 changes: 5 additions & 0 deletions ...@lwc/integration-karma/test/static-content/x/staticUnsafeTopLevel/staticUnsafeTopLevel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { LightningElement, api } from 'lwc';

export default class extends LightningElement {
@api doRender = false;
}
27 changes: 14 additions & 13 deletions ...ages/@lwc/template-compiler/src/__tests__/fixtures/static-content/no-th-or-td/expected.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,22 @@
import { registerTemplate } from "lwc";
import { parseFragment, registerTemplate } from "lwc";
const $fragment1 = parseFragment`<th${3}></th>`;
const $fragment2 = parseFragment`<td${3}></td>`;
const stc0 = {
key: 0,
};
const stc1 = {
key: 1,
};
const stc2 = {
key: 3,
};
const stc3 = {
key: 4,
};
const stc4 = {
key: 6,
key: 5,
};
function tmpl($api, $cmp, $slotset, $ctx) {
const { k: api_key, h: api_element, i: api_iterator } = $api;
const {
k: api_key,
st: api_static_fragment,
h: api_element,
i: api_iterator,
} = $api;
return [
api_element("table", stc0, [
api_element(
Expand All @@ -27,20 +28,20 @@ function tmpl($api, $cmp, $slotset, $ctx) {
{
key: api_key(2, row.id),
},
[api_element("th", stc2)]
[api_static_fragment($fragment1(), 4)]
);
})
),
api_element(
"tbody",
stc3,
stc2,
api_iterator($cmp.rows, function (row) {
return api_element(
"tr",
{
key: api_key(5, row.id),
key: api_key(6, row.id),
},
[api_element("td", stc4)]
[api_static_fragment($fragment2(), 8)]
);
})
),
Expand Down
8 changes: 1 addition & 7 deletions packages/@lwc/template-compiler/src/codegen/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import * as astring from 'astring';
import { isBooleanAttribute, SVG_NAMESPACE, LWC_VERSION_COMMENT } from '@lwc/shared';
import { generateCompilerError, TemplateErrors } from '@lwc/errors';

import { isUnsafeTopLevelSerializableElement } from '../parser/tag';
import {
isComment,
isText,
Expand Down Expand Up @@ -72,12 +71,7 @@ function transform(codeGen: CodeGen): t.Expression {
const databag = elementDataBag(element, slotParentName);
let res: t.Expression;

if (
codeGen.staticNodes.has(element) &&
isElement(element) &&
// Tags like <th> and <td> are okay as static fragments, but only if they're not at the top level
!isUnsafeTopLevelSerializableElement(element)
) {
if (codeGen.staticNodes.has(element) && isElement(element)) {
// do not process children of static nodes.
return codeGen.genHoistedElement(element, slotParentName);
}
Expand Down
20 changes: 0 additions & 20 deletions packages/@lwc/template-compiler/src/parser/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,23 +186,3 @@ export const ATTR_NAME = {
XLINK_HREF: 'xlink:href',
};
export const TEMPLATE_DIRECTIVES = [/^key$/, /^lwc:*/, /^if:*/, /^for:*/, /^iterator:*/];

// Tags that cannot be parsed at the top level of a fragment. Generated from
// https://github.com/sindresorhus/html-tags/blob/95dcdd5/index.js
// using the test:
// document.createRange().createContextualFragment(`<${tag}></${tag}>`).firstChild === null
// TODO [#2890]: use <template> instead, drop IE11 support for static optimization
export const TAGS_THAT_CANNOT_BE_PARSED_AS_TOP_LEVEL = new Set([
HTML_TAG.BODY,
HTML_TAG.CAPTION,
HTML_TAG.COL,
HTML_TAG.COLGROUP,
HTML_TAG.HEAD,
HTML_TAG.HTML,
HTML_TAG.TBODY,
HTML_TAG.TD,
HTML_TAG.TFOOT,
HTML_TAG.TH,
HTML_TAG.THEAD,
HTML_TAG.TR,
]);
13 changes: 0 additions & 13 deletions packages/@lwc/template-compiler/src/parser/tag.ts
View file
Open in desktop

This file was deleted.