WebReflection / hyperHTML

A Fast & Light Virtual DOM Alternative
ISC License
3.07k stars 113 forks source link

Form can not have element with id of 'remove' #383

Closed atirip closed 4 years ago

atirip commented 4 years ago
hyperHTML.bind(document.body)`
    <form id="form" hidden></form>
    <button id="remove" form="form" name="button" type="submit">foo</button>
    <form id="form2" hidden></form>
    <button id="foo" form="form2" name="button" type="submit">foo</button>
`;
console.log(document.getElementById('form').remove);
console.log(document.getElementById('form2').remove);

   <button id="remove" form="form" name="button" type="submit">foo</button>
  hyper-test.js:46 ƒ remove() { [native code] }

Later, when that form node needs to be dropped:

var drop = function drop(node) {
    return (node.remove || dropChild).call(node);
};

that crashes, as node.remove is node and not function

WebReflection commented 4 years ago
  1. this is eventually a domdiff bug
  2. you are shadowing the native Element.prototype.remove, so you are causing yourself a side effect into regular elements capabilities ... is there any specific reason to do that? 'cause append would fail as well, and I'm not sure how this could be solved without degrading performance for everyone else that wouldn't pollute the global scope with a remove leaked node (IDs leaks on the global scope) but also if you pass the form around, form.remove() will fail too.
atirip commented 4 years ago

No specific reason, just haven't been bitten by that before. Calling a button 'remove' is hardly a weird choice :-) I can easily change that to 'erase'. This is pretty sharp edge case, probably just 'form' tag weirdnesses needs to be warned in documentation or similar?

WebReflection commented 4 years ago

probably just 'form' tag weirdnesses needs to be warned in documentation or similar?

to be honest, if you have a form with its inputs and these have names, form.name would also shadow the native prototype.

It is definitively an edge case, but remove is not an uncommon word, so I better write this in the documentation as caveat when it comes to forms, as I don't think having defensive code in comdiff core would be too beneficial in the wild.

Will create a section in either the README or the official documentation (or both).

WebReflection commented 4 years ago

For reference: https://twitter.com/niksy/status/1204768742330322944

I think this is a won't fix, as it's clear using bad names can lead to any sort of footgun, so I'll close it as won't fix and will put a note somewhere about this.

atirip commented 4 years ago

Agree. One suggestion though: maybe a specific error message? Like to check whether it is function you call and if not, then specific message. Ot try/catch. But as you can witness, only now I realise that libraries may need 'remove' and I have been doing this for quite many years :-)

WebReflection commented 4 years ago

if I put try/catches around I am penalizing critical core performance for an edge case, same if I touch that by an inch (using typeof or anything else).

The thing is, anyone could overwrite any DOM native method and I don't want to be defensive about that, so remove is not a special case, as even changing addEventListener in a node would faile all over the place with 3rd parts libraries.