Fix how read-only DOM attributes are set

[jake-low]

Hello!

I've been using Crank for a small project (delightful, by the way!) and found a bug with how SVG attributes are being set during DOM patching. I discovered this bug when I wrote code like this:

renderer.render(
  <svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20">
    <rect x="0" y="0" width="10" height="20" class="foo" data-bar="1" />
  </svg>
);

and found that this didn't produce the desired DOM state, but instead something like this:

<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20">
  <rect class="foo" data-bar="1"></rect>
</svg>

where the x, y, width, and height attributes are missing.

I read the patch implementation and figured out the cause. The intent of the code as currently written appears to be this: try writing to the node property (node[name] = value), and if that throws an error, fall back to setting as an attribute (node.setAttribute(name, value)).

However, it seems based on my ad-hoc testing, attempting to set a read-only property on a DOM element (either an HTMLElement like <p> or an SVGElement like <rect>) doesn't actually throw an error; rather the assigned value is silently discarded.

let p = document.createElement("p");
p.id = "foo"; // id is a writable property
console.log(p.id); // => foo

p.isContentEditable = true; // isContentEditable is a read-only property
console.log(p.isContentEditable); // => false

// same story but for SVG
let rect = document.createElementNS("http://www.w3.org/2000/svg", "rect")
console.log(rect.x);
// => SVGAnimatedLength {
// animVal: SVGLength {unitType: 1, value: 0, valueInSpecifiedUnits: 0, valueAsString: '0'}
// baseVal: SVGLength {unitType: 1, value: 0, valueInSpecifiedUnits: 0, valueAsString: '0'}
// }
rect.x = 10; // x is read-only
console.log(rect.x); // same as above

The solution I came up with is this: instead of trying to set the property and falling back to setAttribute on error, instead walk up the object's prototype chain to find the owner of the property, and then get the descriptor of the property and use that to check if it's writable or not.

I added some additional test assertions for this, but these tests wouldn't actually have caught the bug, because JSDOM appears to permit writing to any Element property.

Feel free to let me know if you'd prefer a different approach; I'm happy to rework the code if you have feedback.

Thanks for your time, and for sharing Crank! It's really been a pleasure to use, and I really appreciate the care that was put into the docs and the motivation blog post.

[jake-low]

Looks like I used some syntax that wasn't supported in Node 10; should be fixed now.

[brainkim]

Shipped in 0.4.3. Thanks again!