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

svg_element function is output even if no svg element in a component #6555

Closed
tomoam opened this issue Jul 22, 2021 · 2 comments · Fixed by #6556
Closed

svg_element function is output even if no svg element in a component #6555

tomoam opened this issue Jul 22, 2021 · 2 comments · Fixed by #6556
Labels

Comments

@tomoam
Copy link
Contributor

tomoam commented Jul 22, 2021

Describe the problem

when hydratable: true, svg_element function is output even if no svg element in a component.

For example, if you have a simple component like the following:

<main>
	<h1>Hello world!</h1>
</main>

compiled:

  function svg_element(name) {
    return document.createElementNS("http://www.w3.org/2000/svg", name);
  }

  ...

  function claim_element(nodes, name, attributes, svg) {
    return claim_node(
      nodes,
      (node) => node.nodeName === name,
      (node) => {
        const remove = [];
        for (let j = 0; j < node.attributes.length; j++) {
          const attribute = node.attributes[j];
          if (!attributes[attribute.name]) {
            remove.push(attribute.name);
          }
        }
        remove.forEach((v) => node.removeAttribute(v));
      },
      () => (svg ? svg_element(name) : element(name))
    );
  }

minified(formatted):

  function m(t, n, e, o) {
    return _(
      t,
      (t) => t.nodeName === n,
      (t) => {
        const n = [];
        for (let o = 0; o < t.attributes.length; o++) {
          const r = t.attributes[o];
          e[r.name] || n.push(r.name);
        }
        n.forEach((n) => t.removeAttribute(n));
      },
      () =>
        o
          ? (function (t) {
              return document.createElementNS("http://www.w3.org/2000/svg", t);
            })(n)
          : s(n)
    );
  }

This isn't a bug (because it works fine), but it does increase the bundle size slightly.

Describe the proposed solution

Pass svg_element function, instead of 1.

  • src/compiler/compile/render_dom/wrappers/Element/index.ts
	get_claim_statement(nodes: Identifier) {
		const attributes = this.attributes
			.filter((attr) => !(attr instanceof SpreadAttributeWrapper) && !attr.property_name)
			.map((attr) => p`${(attr as StyleAttributeWrapper | AttributeWrapper).name}: true`);

		const name = this.node.namespace
			? this.node.name
			: this.node.name.toUpperCase();

-		const svg = this.node.namespace === namespaces.svg ? 1 : null;
+		const svg = this.node.namespace === namespaces.svg ? '@svg_element' : null;

		return x`@claim_element(${nodes}, "${name}", { ${attributes} }, ${svg})`;
	}
  • src/runtime/internal/dom.ts
-	export function claim_element(nodes: ChildNodeArray, name: string, attributes: { [key: string]: boolean }, svg) {
+	export function claim_element(nodes: ChildNodeArray, name: string, attributes: { [key: string]: boolean }, create_element: (name: string) => Element | SVGElement = element) {
		return claim_node<Element | SVGElement>(
			nodes,
			(node: ChildNode): node is Element | SVGElement => node.nodeName === name,
			(node: Element) => {
				const remove = [];
				for (let j = 0; j < node.attributes.length; j++) {
					const attribute = node.attributes[j];
					if (!attributes[attribute.name]) {
						remove.push(attribute.name);
					}
				}
				remove.forEach(v => node.removeAttribute(v));
				return undefined;
			},
-			() => svg ? svg_element(name as keyof SVGElementTagNameMap) : element(name as keyof HTMLElementTagNameMap)
+			() => create_element(name)
		);
	}

Alternatives considered

Also, element function is output even if the component has only svg elements.
If it is important to improve that as well, more changes are needed.

Importance

nice to have

@tomoam
Copy link
Contributor Author

tomoam commented Jul 27, 2021

In response to #6556 comments, I've thought of several implementations to fix #6555, but each has its trade-offs.

  • impl1
    • same as #6556
    • This allows to exclude svg_element function if a component does'nt have svg-elements, but not exclude element function if a component only includes svg-elements.
  • impl2
    • This allows to exclude svg_element function if a component does'nt have svg-elements, and exclude element function if a component only includes svg-elements.
    • Each elements pass element function or svg_element function to claim_element function in claim function.
      • compiled
          l: function claim(nodes) {
              ...
              main = claim_element(nodes, "MAIN", {}, element);
              ...
          },
      • minified(formatted)
          l(t) {
              ...
              e = $(t, "MAIN", {}, s);
              ...
          },
    • Trade-off
      The size increases by 2 bytes for each claim_element, so if there are few elements, the size will be small, but if there are many elements, the size will be large.
  • impl3
    • This allows to exclude svg_element function if a component does'nt have svg-elements, and exclude element function if a component only includes svg-elements.
    • renamed claim_element to claim_element_base, and created wrapper functions.
      -   export function claim_element(nodes: ChildNodeArray, name: string, attributes: {[key: string]: boolean}, svg) {
      +   export function claim_element_base(nodes: ChildNodeArray, name: string, attributes: { [key: string]: boolean }, create_element: (name: string) => Element | SVGElement) {
              ...
          }
      
      +   export function claim_element(nodes: ChildNodeArray, name: string, attributes: { [key: string]: boolean }) {
      +       return claim_element_base(nodes, name, attributes, element);
      +   }
      
      +   export function claim_svg_element(nodes: ChildNodeArray, name: string, attributes: { [key: string]: boolean }) {
      +       return claim_element_base(nodes, name, attributes, svg_element);
      +   }
    • Trade-off
      If a component includes both svg-elements and not-svg-elements, and the number of elements is small, the footprint can be large.

To clarify the trade-offs of these implementations, I compiled the following components in each implementation and compared the bundle sizes.

  • Small-size components
    • a component not including svg-elements.
    • a component including only svg-elements.
    • a component including both.
  • Middle-size components
    • a component not including svg-elements.
    • a component including only svg-elements.
    • a component including both.

The sources and artifacts used for the comparison can be found in this repository.
https://github.com/tomoam/svelte-benchmark-for-6555.git

Results

Small-size components

Component

v3.40.0 impl1 impl2 impl3
HelloWorld minified 5338 🥇 5258 🥈 5260 🥉 5289
HelloWorld minified gzip 2308 2268 2267 2272
HelloWorld minified brotli 2087 2055 2054 2052
RoundedRectangle minified 5333 🥉 5331 🥇 5282 🥈 5311
RoundedRectangle minified gzip 2304 2300 2290 2293
RoundedRectangle minified brotli 2085 2080 2078 2079
HelloWorld and RoundedRectangle minified 🥉 6487 🥇 6482 🥈 6484 6548
HelloWorld and RoundedRectangle minified gzip 2693 2688 2687 2706
HelloWorld and RoundedRectangle minified brotli 2414 2411 2409 2424

Note: The sizes for gzip and brotli are for reference only.

Middle-size components

Component

v3.40.0 impl1 impl2 impl3
TodoMVC minified 12854 🥇 12774 🥉 12820 🥈 12805
TodoMVC minified gzip 5198 5160 5159 5164
TodoMVC minified brotli 4602 4573 4578 4577
Icons minified 🥉 14023 14025 🥈 13976 🥇 13877
Icons minified gzip 4567 4559 4554 4557
Icons minified brotli 3922 3928 3934 3919
TodoMVC and Icons minified 🥉 22560 🥈 22559 22605 🥇 22500
TodoMVC and Icons minified gzip 7767 7763 7766 7770
TodoMVC and Icons minified brotli 6710 6700 6718 6706

Note: The sizes for gzip and brotli are for reference only.

Comparison of implementations

v3.40.0 and impl1

In almost all cases, the bundle size of impl1 is smaller than that of v3.40.0. There is no doubt that impl1 is better.

impl2 and impl3

This depends on the use case. If the number of elements is small, the bundle size of impl2 will be small, but if the number of elements is large, the bundle size of impl3 will be smaller.
According to my calculations, if the total number of elements in the application exceeds 31, the bundle size of impl3 will be smaller. Personally, impl3 seems to be the better choice.

impl1 and impl3

This also depends on the use case.In many cases, impl3 can reduce the bundle size, but if you use svg-elements and not-svg-elements to create components with only a few elements, the larger footprint of impl3 will be noticeable. However, if the component is to be used in a Svelte application, impl3 is a better choice. I think that use case is more common.

Based on these, I am going to submit a PR with impl3. I would appreciate your opinion.

Sorry for my English.

@Conduitry
Copy link
Member

This should be fixed in 3.42.2 - the element and svg_element helpers should only be getting included when hydratable components contain those types of elements.

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

Successfully merging a pull request may close this issue.

2 participants