Indistinguishable properties and attributes

[mindplay-dk]

Currently, the patch() function treats every JSX attribute simultaneously as a property and an attribute - it initializes both, which means you have no ability to update them individually.

Blindly treating every JSX attribute as both a property and an attribute means that most updates are unnecessarily being done twice - one of them generating either a meaningless property or a meaningless DOM attribute. The DOM accumulates a lot of garbage this way.

For example, <div innerHTML={content}> will inject literal HTML via the innerHTML property, but will also create a useless innerHTML attribute - or in some cases, such as <input value={value}>, will initialize the value twice, first using the value property, then again using setAttribute('value', ...).

I don't have a lot of references, but snabbdom for one separates properties from attributes - which seems sort of inevitable, at the VDOM level, if we want to solve this problem?

[jorgebucaran]

@mindplay-dk I don't use innerHTML and I can't recommend using it. But if I really had to, I would not do it declaratively, using h or JSX, but inside a lifecycle event.

What about Preact/React, I don't recall they having any special syntax for distinguishing attributes and props and I think that's great, because it makes it easier to reason about my user interface. As a user, I don't really want to think about attributes and props as separate entities. This was a particular pain point with Snabbdom, when I used it, before Hyperapp/Picodom.

[mindplay-dk]

What about Preact/React, I don't recall they having any special syntax for distinguishing attributes and props and I think that's great, because it makes it easier to reason about my user interface

Yes, but I think this is one of the reasons why these frameworks are considerably larger and more complex - they make distinctions between different element-types and their attributes.

For example, React use code like this to detect specific element types:

https://github.com/facebook/react/blob/45c1ff348e1c7d03567f5bba6cb32cffa9222972/packages/shared/isTextInputElement.js

They have to make distinctions and run-time, and that requires knowledge of every element type, it's properties and attributes - in fact, they make numerous distinctions beyond just properties and attributes, here:

https://github.com/facebook/react/blob/94f44aeba72eacb04443974c2c6c91a050d61b1c/packages/react-dom/src/shared/DOMProperty.js#L43

Properties: object mapping DOM property name to one of the
DOMPropertyInjection constants or null. If your attribute isn't in here,
it won't get written to the DOM.

It's not unthinkable that we can find a much simpler way to deal with this, but it most likely will need to include either a blacklist or whitelist of some sort?

[jorgebucaran]

@mindplay-dk I don't think Preact could afford that.

[jorgebucaran]

@mindplay-dk You already gave one good example, innerHTML. I already expressed my opinion on that property and here is something I wrote about sanitation.

What are other props/attributes examples that you think should be treated differently?

[mindplay-dk]

There are interesting edge-cases - for example, if you do readOnly={true}, that's the property, whereas readonly={true} is the attribute, which needs a string cast. They're not even the same type.

Custom properties don't map to attributes:

document.querySelector("input").foo = "bar"
> "bar"
document.querySelector("input").setAttribute("foo", "qux")
document.querySelector("input").foo
> "bar"

But many built-in properties do map to an attribute:

document.querySelector("input").id = "aaa"
> "aaa"
document.querySelector("input").setAttribute("id", "bbb")
document.querySelector("input").id
> "bbb"

So, some properties update their attribute counterpart as a side-effect, and vice-versa. Fun times.

I wonder if we can exploit this somehow?

[mindplay-dk]

I don't think Preact could afford that

Sure - it can do everything at compile-time.

[mindplay-dk]

Here's the relevant portion of the Preact sources for reference:

        if (name!=='list' && name!=='type' && !isSvg && name in node) {
		setProperty(node, name, value==null ? '' : value);
		if (value==null || value===false) node.removeAttribute(name);
	}
	else {
		let ns = isSvg && (name !== (name = name.replace(/^xlink\:?/, '')));
		if (value==null || value===false) {
			if (ns) node.removeAttributeNS('http://www.w3.org/1999/xlink', name.toLowerCase());
			else node.removeAttribute(name);
		}
		else if (typeof value!=='function') {
			if (ns) node.setAttributeNS('http://www.w3.org/1999/xlink', name.toLowerCase(), value);
			else node.setAttribute(name, value);
		}
	}

It's doing something like what I suggested - it's checking for name in node (with some more criteria) and doesn't update the attribute if it finds that this is a property. But to deal with id and other edge-cases (as I explained above) they also removeAttribute() if the value is null (ish) to avoid accumulating garbage like id="" in the document model itself.

I'm experimenting with something similar, I'll let you know what I find...

[mindplay-dk]

I'm close to a solution, but the test-framework seems to contain an inconsistency with real browsers.

In a real brower, setting the element.value property also updates the attribute.

In seems, in the test framework, that's not the case.

In fact, there's a passing test in the current test-suite, which I think should actually fail:

testTrees("update element with dynamic props", [
  {
    node: h("input", {
      type: "text",
      oncreate(element) {
        element.value = "bar"
      },
      value: "foo"
    }),
    html: `<input type="text" value="foo">`
  },

Here, the test says that the initialization via props value: "foo" should win - but oncreate gets called last, and if I change the test to use setAttribute instead, it fails:

testTrees("update element with dynamic props", [
  {
    node: h("input", {
      type: "text",
      oncreate(element) {
        //element.value = "bar"
        element.setAttribute("value", "bar")
      },
      value: "foo"
    }),
    html: `<input type="text" value="foo">`
  },

oncreate executes after property initializations, so the value should be "bar", right?

But setting the value property doesn't update the attribute.

Try setting element.value in the property and then getAttribute("value") - it works, but with the test-framework, it doesn't appear to.

I'm reluctant to change the test before checking this with you, but I think the test is wrong, and it's passing because of a bug in the test-framework?

So I have two failing tests as of right now:

  ● update element with dynamic props

    expect(received).toBe(expected)

    Expected value to be (using ===):
      "<input type=\"text\" value=\"foo\">"
    Received:
      "<input type=\"text\">"

      at test/vdom.test.js:9:39
      at Array.forEach (native)
      at Object.<anonymous> (test/vdom.test.js:7:11)
      at process._tickCallback (internal/process/next_tick.js:109:7)

  ● elements with falsy values

    expect(received).toBe(expected)

    Expected value to be (using ===):
      "<input type=\"text\" value=\"\">"
    Received:
      "<input type=\"text\">"

      at test/vdom.test.js:9:39
      at Array.forEach (native)
      at Object.<anonymous> (test/vdom.test.js:7:11)
      at process._tickCallback (internal/process/next_tick.js:109:7)

The first is what I just explained.

The second one, I think, is actually correctly omitting the value="" attribute, since it's empty, and we've decided we don't want empty attributes such as id="" per another part of the spec. But I wanted to check this with you as well before changing the spec.

[jorgebucaran]

@mindplay-dk Please don't worry so much about tests for now. If you have a working PR addressing the issues you were having, I'd like to see it first. That way I can understand what this issue is really about, because I have to confess that I am still confused.

Or perhaps I am just not that concerned about 100% accuracy, as long as "it works". Show me the code! 💸 😄

[mindplay-dk]

It's on this branch :-)

[jorgebucaran]

@mindplay-dk Do you think it would be possible to refactor this part:

if (null == value || false === value) {
  element.removeAttribute(name)
}

...so that it is not repeated, but reused for both branches.

[mindplay-dk]

Yes, though this requires the introduction of a var empty for the test result.

[mindplay-dk]

What do you think about the failing tests?

Do you agree that this might be a bug in the test-framework? From what I could find, this is not how a real browser behaves - but I honestly have very little experience with testing in JS, and I'm not even sure which package or component is at fault.

[jorgebucaran]

@mindplay-dk Sorry, I was not aware of any failing tests. If you can send me a PR with these changes so we can review it, please do.

[mindplay-dk]

There are two failing tests, as explained above - the first seems like it should have failed all along (before I changed anything) but may be passing due to a bug in the test-framework. I'm really not confident enough to say that with certainty.

And as explained, because of the same bug, I can't make the other test pass - although I'm fairly certain it would pass in a real browser.

As said, I don't know enough about this test-framework to even say which package is at fault.

So I'm kind of stuck with both of these :-/

[jorgebucaran]

@mindplay-dk I have a question: are you treating values as properties only if the key is name, or am I reading the code wrong?

[mindplay-dk]

I'm treating values as properties if they map to properties ;-)

let foo = { bar: "baz" }
let name = "bar"
name in foo // true

[jorgebucaran]

@mindplay-dk I was reading it wrong. Thanks! This looks good, could you make a PR?

[mindplay-dk]

Yeah, but... I have two failing tests and no idea how to fix either of them.

[jorgebucaran]

Well, in that case I guess I'll help myself with your fork and try to come up with a solution myself! 💪😄

[mindplay-dk]

An update on this: the test-framework isn't wrong, the tests are!

The assertions for these tests are wrong:

  ● update element with dynamic props

    expect(received).toBe(expected)

    Expected value to be (using ===):
      "<input type=\"text\" value=\"foo\">"
    Received:
      "<input type=\"text\">"

      at test/vdom.test.js:9:39
      at Array.forEach (native)
      at Object.<anonymous> (test/vdom.test.js:7:11)
      at process._tickCallback (internal/process/next_tick.js:109:7)

  ● elements with falsy values

    expect(received).toBe(expected)

    Expected value to be (using ===):
      "<input type=\"text\" value=\"\">"
    Received:
      "<input type=\"text\">"

      at test/vdom.test.js:9:39
      at Array.forEach (native)
      at Object.<anonymous> (test/vdom.test.js:7:11)
      at process._tickCallback (internal/process/next_tick.js:109:7)

That is, we are getting the expected innerHTML - it's not supposed to include the value attribute, because we're no longer setting the value-attribute, we're setting the value property instead, which, in a normal browser, does not update the attribute.

For certain other attributes such as id, setting the property does set the attribute, for the value it does not, because the value attribute in the document is only used to initialize the state of the input object - it doesn't map two-ways with the value of the input object itself.

I'll see if I can correct the tests.

[mindplay-dk]

Bingo, the tests are passing 😄

I've opened a PR which I think is ready for review now.