tbranyen/diffhtml

delete in removeAttribute causes TypeError

CetinSert opened this issue · 7 comments

CetinSert commented

Please note that in the following only the second delete is in a try catch:

diffhtml/packages/diffhtml/lib/node/patch.js

Lines 87 to 105 in cab7fb3

const removeAttribute = (vTree, domNode, name) => {
// Runtime checking if the property can be set.
const blocklistName = vTree.nodeName + '-' + name;
const anyNode = /** @type {any} */ (domNode);
if (allowlist.has(blocklistName)) {
anyNode[name] = undefined;
delete anyNode[name];
}
else if (!blocklist.has(blocklistName)) {
try {
anyNode[name] = undefined;
delete anyNode[name];
allowlist.add(blocklistName);
}
catch {
blocklist.add(blocklistName);
}
}

The first delete in

diffhtml/packages/diffhtml/lib/node/patch.js

Line 94 in cab7fb3

delete anyNode[name];

causes https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cant_delete exceptions for some web components that implement attribute ↔ property reflection with Object.defineProperty.

tbranyen commented

Can you post an example of what is breaking? The first delete is not in a try/catch intentionally for performance reasons. This code attempts to delete a property and if it succeeds, marks that property as safe for a given tag name. That's the only way the first delete will be reached.

  • 👍1
CetinSert commented

Here is an absolutely minimal case: https://tw.uido.co/test/index.html

<script src="//webpdf.co/<>" type=module></script>

<pdf-file id=f src=//pdf.ist/web.pdf></pdf-file>
<div      id=i><pdf-page off=f></pdf-page></div>

<script type=module>
  import * as diff from '//esm.run/diffhtml'; window.diff = diff;
  await customElements.whenDefined('pdf-page');
  await diff.innerHTML(i, `<pdf-page of=f></pdf-page>`);  // patch.js     setAttribute  does allowlist.add(blocklistName)
  await diff.innerHTML(i, `<pdf-page off=f></pdf-page>`); // patch.js removeAttribute checks allowlist TypeError
</script>
CetinSert commented

The of attribute is defined using Object.defineProperty (with configurable set to false; as is default).

So of

setAttribute and removeAttribute would need different

  • allowlists
    or
  • blocklistNames

for HTML custom elements that define attribute ↔ property mirroring via Object.defineProperty.

CetinSert commented

@tbranyen – I have demoed and described the problem/solution in the above 2 comments.

tbranyen commented

Fix opened here @CetinSert #262

  • 👍1
  • ❤️1
  • 🚀1
tbranyen commented

Darnit, so my Travis CI account just ran out of open source credits. The credits should reset in an hour and a half when the new month starts. Would be awesome to convert this project to GitHub Actions CI infra instead.