perf: static-optimize xlink:href
#4346
Conversation
| const value = | ||
| isStringLiteral(attrValue) && !isFragmentOnlyUrl(attrValue.value) | ||
| ? t.literal(attrValue.value) | ||
| : codeGen.genScopedFragId(expression); |
There was a problem hiding this comment.
This is a little kooky to me – basically we only add the api_scoped_frag_id() function when the value is a non-literal or starts with a #:
// dynamic
api_scoped_frag_id($cmp.foo)
// static, starts with #
api_scoped_frag_id('#yolo')
// static, does not start with #
'/foo.svg'We could just run everything through the api_scoped_frag_id function, since it already checks to see if the value starts with a #. But I guess this is an optimization for static values that don't start with #. So I preserved the existing behavior.
| @@ -744,9 +748,13 @@ export default class CodeGen { | |||
| // TODO [#3658]: `disableSyntheticShadowSupport` should also disable this dynamic behavior | |||
| const isIdOrIdRef = | |||
| (name === 'id' || isIdReferencingAttribute(name)) && | |||
| (isExpression(value) || isStringLiteral(value)); | |||
| !isBooleanLiteral(value); | |||
There was a problem hiding this comment.
The only three possibilities here are 1) expression, 2) string literal, 3) boolean literal.
| @@ -9,32 +9,22 @@ import { | |||
| sanitizeAttribute, | |||
| } from "lwc"; | |||
| const $fragment1 = parseFragment`<a class="test${0}" data-foo="datafoo" aria-hidden="h" role="presentation" href="/foo" title="test" tabindex="-1"${2}></a>`; | |||
| const $fragment2 = parseFragment`<table bgcolor="x"${3}></table>`; | |||
| const $fragment3 = parseFragment`<div${"c0"} aria-hidden="hidden"${2}></div>`; | |||
| const $fragment2 = parseFragment`<svg class="cubano${0}" focusable="true"${2}><use${"a1:xlink:href"}${3}/></svg>`; | |||
There was a problem hiding this comment.
@nolanlawson - The assumption in buildSerializeExpressionFn is that the first : in an attribute is the delimiter to split the part tokens.
Do you think it would be worth adding a comment in the buildSerializeExpressionFn that the attribute name can contain a : in it as well?
| const $fragment1 = parseSVGFragment`<defs${3}><circle${"a1:id"} r="10" cx="10" cy="10" fill="black"${3}/><circle${"a2:id"} r="10" cx="14" cy="14" fill="red"${3}/></defs>`; | ||
| const stc0 = { | ||
| attrs: { | ||
| width: "100px", | ||
| height: "100px", | ||
| viewport: "0 0 100 100", | ||
| }, | ||
| key: 0, | ||
| svg: true, | ||
| }; | ||
| const $fragment1 = parseFragment`<svg width="100px" height="100px" viewport="0 0 100 100"${3}><defs${3}><circle${"a2:id"} r="10" cx="10" cy="10" fill="black"${3}/><circle${"a3:id"} r="10" cx="14" cy="14" fill="red"${3}/></defs><use${"a4:href"}${3}/><use${"a5:xlink:href"}${3}/></svg>`; |
There was a problem hiding this comment.
Will using parseSVGFragment vs parseFragment cause any observable changes?
I'm not sure if I'm interpreting this correctly but it looks like with parseSVGFragment the content that's returned is just the <defs> whereas with parseFragment the content will be returned with a wrapping <svg> tag.
If it does, will it break any existing tests?
There was a problem hiding this comment.
This is expected. For the <svg> element itself, we always do parseFragment, not parseSVGFragment, due to this logic:
lwc/packages/@lwc/template-compiler/src/codegen/codegen.ts
Lines 625 to 628 in 17c990d
The reason this changed in the above fixture is because we are static-optimizing the whole <svg> rather than the <defs> now.
I think the reason we did it this way is because <svg> automatically forces the HTML parser to use the SVG namespace, but for the stuff inside, we have to explicitly explain that it's in the SVG namespace.
There was a problem hiding this comment.
Here's a CodePen to demonstrate. Note that the <circle> only works when parsed within the <svg>, not by itself.
| this, | ||
| // `addLegacySanitizationHook` is true because `isCustomRendererHookRequired` | ||
| // being false is a precondition for static nodes. | ||
| true |
There was a problem hiding this comment.
FYI @jmsjtu I added this bit here. I realized that due to the isCustomRendererHookRequired check here, this is always true:
lwc/packages/@lwc/template-compiler/src/codegen/static-element.ts
Lines 122 to 125 in 275218a
Details
Partially addresses #4280
Adds support in the static content optimization for
xlink:hrefandhrefwhen used in an SVG<use>.Also adds some tests for uncovered test cases (e.g. boolean-true:
<use xlink:href>).Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?