perf: special handling for static elements

[jodarove]

Details

Perf

The regressions are from 1k-different-style tests, and are because the component under test template is static, and it needs to replace the style token.

Does this pull request introduce a breaking change?

• ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

• ⚠️ Yes, it does include an observable change.

Because class and style attributes of static nodes are not parsed into classMap and styleMap, it may be the case that they may contain extra characters in ssr and the dom. ex: <div class=" foo bar"> now generates exactly that, while before this pr it used to generate <div class="foo bar">

[caridy]

this property should probably be renamed to frag or fragment, using Proto postfix is misleading.

[caridy]

why not reusing VElementData here? and take the key out of the data? My hunch is that eventually we might be able to do some crazy optimizations where the data payload can provide some stuff for the subtree, so it makes sense to keep it around.

[jodarove]

atm is not needed and this is internals that we could modify to add the data bag once we have an optimization in mind. passing the key directly will reduce size (fewer chars), and perf (destructing key from the databag). however, I can generate the databag if that is preferred.

[caridy]

Looking good... simpler that I thought it was going to be. Minor details to figure out and some other silly questions :)

[jodarove]

Note: I don't like the extra ""="", or the class="". what's your opinion about doing about stripping them away? at least in dev or test.

Notice that we can fix the errors in our repo, but I figure that there will be other repos with tests in the same situation.

Alternatively, what about doing our own serializer (sounds difficult)? to work around the fact that parse5 attributes value always needs to be defined (we can't generate `<div ${1}>).

[caridy]

I hope that we can avoid ""="", or the class="", let's chat

[jodarove]

resolved in 706a6fd (adds a custom serializer for static elements using our ast)

[nolanlawson]

My intuition tells me that it would be faster at runtime to create elements and add attributes (i.e. document.createElement, appendChild, setAttribute, etc.), rather than use createContextualFragment. It would avoid parsing HTML on the client-side.

However, the tradeoff is that the current approach has a more compact code representation. And for very large static content (e.g. lots of nested elements), I think creating the fragment may actually be faster at runtime. So this seems like a good approach to me.

[nolanlawson]

Reviewers note: the snapshot tests have not been updated to ease up the review.

FWIW, I usually just use the "file filter" in the GitHub UI to filter out .js files so I don't see the snapshots. Typically our source files are only .ts, not .js.

[nolanlawson]

This looks reasonable to me; it feels a bit simpler and more straightforward than the other PR. I'm curious to know what the perf improvement looks like?

Nice work!

[nolanlawson]

Confirmed that 95ce4c3 broke the test in this PR.

[nolanlawson]

Did an analysis of downstream Nucleus tests. Seems there are only 2 new failures, and they are due to minor snapshot differences.

[nolanlawson]

Blocked by #2859.

[nolanlawson]

Looks like this PR was severely bitrotted by da49079. Merge conflicts galore.

In particular, this part seems problematic now:

lwc/packages/@lwc/engine-core/src/framework/template.ts

Lines 180 to 186 in 4d5b57e

	// Note: at the moment this code executes, createFragment have not being set.
	export const parseFragment = buildParseFragmentFn((html) => createFragment(html));
	export const parseSVGFragment = buildParseFragmentFn((html) => {
	const fragment = createFragment('<svg>' + html + '</svg>');
	return getFirstChild(fragment);
	});

We don't have access to the renderer anymore in @lwc/engine-core, so I'm not sure how this is supposed to work anymore.

[caridy]

why do we reuse the same parseFragment in server?

[nolanlawson]

It looks like the reason is because parseSVGFragment uses getFirstChild, which is not supported in the server environment:

lwc/packages/@lwc/engine-core/src/framework/template.ts

Lines 182 to 186 in 4d5b57e

	export const parseSVGFragment = buildParseFragmentFn((html) => {
	const fragment = createFragment('<svg>' + html + '</svg>');
	return getFirstChild(fragment);
	});

If I try to use the actual parseSVGFragment, I get an error in the Jest tests:

Click to see

Summary of all failing tests
 FAIL  packages/@lwc/engine-server/src/__tests__/fixtures.spec.ts (18.993 s)
  ● fixtures › svgs/index.js

    TypeError: "getFirstChild" is not supported in this environment

      13 |     create,
      14 |     StringToLowerCase,
    > 15 |     htmlPropertyToAttribute,
         |               ^
      16 |     noop,
      17 | } from '@lwc/shared';
      18 |

      at src/renderer.ts:15:15
      at ../engine-core/dist/engine-core.cjs.js:4654:12
      at ../engine-core/dist/engine-core.cjs.js:4645:31
      at tmpl (src/__tests__/fixtures/svgs/dist/compiled.js:35:27)
      at ../engine-core/dist/engine-core.cjs.js:4718:27
      at ReactiveObserver.observe (../engine-core/dist/engine-core.cjs.js:339:13)
      at isUpdatingTemplate (../engine-core/dist/engine-core.cjs.js:4674:13)
      at runWithBoundaryProtection (../engine-core/dist/engine-core.cjs.js:5534:5)
      at evaluateTemplate (../engine-core/dist/engine-core.cjs.js:4667:5)
      at invokeComponentRenderMethod (../engine-core/dist/engine-core.cjs.js:4820:41)

[caridy]

to be removed!

[caridy]

unnecessary abstraction.

[caridy]

to be removed...

[caridy]

I'm not clear why on the server side SVG can just reuse the other one... I don't remember this.

[caridy]

The biggest issue to be resolved here is the fact that, IMO, parsing HTML fragment SHOULD be considered risky all the time... meaning the template SHOULD import the renderer, and call the corresponding parsing method. Today, in this PR, it is not quite like that, what's going on now is the following:

1. if static nodes are detected during compilation of HTML, the corresponding function is imported from LWC (parseFragment or parseSVGFragment), this function is required to be public API from LWC.
2. The result of calling the parse** is used to pass as metadata into a static vnode.
3. the static vnode will carry on other corresponding work to reify the fragment and clone it while using the renderer associated to the vm.

The issue that I see is that the vm will always have what we call the default renderer, meaning locker will not be able to intersect that. This is fine if we make it very clear what can be considered static, but that's more tricky to track down, I prefer to just have an always inspectable parsing mechanism by importing renderer, and calling the corresponding function from renderer that can directly parse without the extra level of abstraction.

cc @ravijayaramappa @nolanlawson

[pmdartus]

Agreed, we should import the rendererer from the engine to let Locker hook itself on it.

I prefer to just have an always inspectable parsing mechanism by importing renderer, and calling the corresponding function from renderer that can directly parse without the extra level of abstraction.

That said, I still think some level of abstraction will be necessary. At least to abstract away the current buildParseFragmentFn. This function is in charge of injecting the scope tokens and caching the fragment.

[nolanlawson]

parseFragment is indeed always risky. As for parseSVGFragment, this could maybe just be the same function with an option for the namespace or something. If we move this to the renderer, then that would satisfy your concerns, right @caridy?

[caridy]

parseFragment is indeed always risky. As for parseSVGFragment, this could maybe just be the same function with an option for the namespace or something. If we move this to the renderer, then that would satisfy your concerns, right @caridy?

That's correct. right now it is not sanitized, but the risk is not that big because LWC compiler parses and validates that HTML, but you never know. I'm fine to do it in another PR :)

[nolanlawson]

Awesome, thanks. Let's merge this, and I opened another issue to track parseFragment/parseSVGFragment: #2876