String objects are rendered as separate characters (each character a separate text node)

[Mehuge]

Which package(s) are affected?

Lit Core (lit / lit-html / lit-element / reactive-element)

Description

When a string is actually a String, LitElement renders each character of the string as a separate text node, surrounded by comment nodes (3 nodes per character)

import {LitElement} from 'lit';
export class SimpleGreeting extends LitElement {
  render = () => (new String('Hello World'))
}
customElements.define("simple-greeting", SimpleGreeting)

The DOM generated for the above is:

I would expect new String("Hello World") to render the same as "Hello World".

We use String quite a lot for language translation.

Reproduction

https://lit.dev/playground/#gist=24662cc3bb88b19084441b6367e6f6ad

Workaround

I can ofc .toString() each String object to avoid this issue. But that's not very convenient and makes the code less readable.

import {LitElement} from 'lit';
export class SimpleGreeting extends LitElement {
  render = () => (new String('Hello World').toString())
}
customElements.define("simple-greeting", SimpleGreeting)

Is this a regression?

No or unsure. This never worked, or I haven't tried before.

Affected versions

Tested in lit-html 3.1.3 & 3.1.4 and lit-element 4.0.5 & 4.0.6

Browser/OS/Node environment

Browser: Chrome Version 125.0.6422.113 (Official Build) (64-bit) OS: Windows 11 Version 10.0.22631 Build 22631

[Mehuge]

I get why it's doing it, because String is iterable, but is that really sensible output for String objects?

Having many single character text nodes must be less performant / use more memory than having a single text node containing the whole string.

[sorvell]

Is there a good reason to use new String? It returns an object which is not a primitive and is not actually a string type (e.g. typeof new String('') === 'object'.

Probably because this is potentially confusing, this isn't generally recommended as MDN notes here.

[Mehuge]

We use String because we mark the result as isTranslated = true which you can't do on a string primitive.

[sorvell]

Another workaround that could make this more ergonomic is to make a little wrapper:

import {html as baseHtml} from 'lit';

const html = (strings, ...values) => baseHtml(strings, 
  ...values.map(value => value instanceof String ? value.toString() : value));  

[justinfagnani]

Detecting the type of value passed to a binding is a very hot code path for us, and I'm a bit afraid that adding another conditional will slow down all of lit-html itself. We'll have to think on this.

---

An aside: I would make render() a plain method. There's no reason for it to be an instance property. Instance properties are allocated per instance and use more memory than methods.

So instead of:

import {LitElement} from 'lit';
export class SimpleGreeting extends LitElement {
  render = () => (new String('Hello World'))
}
customElements.define("simple-greeting", SimpleGreeting)

write:

import {LitElement} from 'lit';
export class SimpleGreeting extends LitElement {
  render() { return new String('Hello World'); }
}
customElements.define("simple-greeting", SimpleGreeting);

[Mehuge]

Another workaround that could make this more ergonomic is to make a little wrapper:

import {html as baseHtml} from 'lit';

const html = (strings, ...values) => baseHtml(strings, 
  ...values.map(value => value instanceof String ? value.toString() : value));  

Thanks for that suggestion, that might give us a workable solution.

[Mehuge]

An aside: I would make render() a plain method. There's no reason for it to be an instance property.

That was just a terse example to demonstrate the problem, we use render() methods normally.