WebReflection / domdiff

Diffing the DOM without virtual DOM
ISC License
214 stars 14 forks source link

The Node.remove call generates side effects #21

Closed GianlucaGuarini closed 4 years ago

GianlucaGuarini commented 4 years ago

The internal call to Node.remove might generate side effects. Can you consider a way to disable it via settings?

WebReflection commented 4 years ago

can you give me an example? the code fallback already to parentNode.removeChild ... maybe this is a duplicate of this one? https://github.com/WebReflection/hyperHTML/issues/383

GianlucaGuarini commented 4 years ago

@WebReflection sure, I have made a simple custom element example https://plnkr.co/edit/uy1wGAxFBp2vN8dKWON3?p=preview

WebReflection commented 4 years ago

so the concept is the same of the other issue I've mentioned, if you overwrite inherited methods, you gonna have issues. If you override removeChild, or append, or any other method in the HTMLElement prototype could have issues.

It's true that domdiff could be defensive and use only Node.prototype trapped methods to work with all nodes, but are you willing to lose performance for that?

kaste commented 4 years ago

Isn't that a general footgun. If you send me that CustomElement, I as a user of your CE have the expectation that I can use it like any other HTMLElement/Node. It is maybe an enhanced version of such an element. Implementing an own remove method which does something completely different is not expected OOP-style programming, is it?

GianlucaGuarini commented 4 years ago

I think the problem is bigger than that. CEs accept attributes that can be converted to Node properties (see demos here https://custom-elements-everywhere.com/). Now if we do something like:

<my-li remove={ someCallback }/>

This will override the Node.prototype.remove. At this point I think this is a platform issue that can not be addressed here :(

WebReflection commented 4 years ago

@kaste exactly my point. append instead of add would break the HTMLELement contract too, and one could also create a parentNode getter that would break the same contract.

This has been discussed already and flagged as wont'f fix, as any method / accessor, once overwritten, can cause side effects in the wild, so it's not a domdiff specific issue.

However, I will one day compare performance trapping primitives that would work no matter what, but I am sure performance will be worse.

Also worth noting, as nobody would create a querySelectorAll overwrite on a generic element, every other method should follow the same rule, imho.

WebReflection commented 4 years ago

@GianlucaGuarini about this:

see demos here https://custom-elements-everywhere.com/

all domdiff based libraries pass that test 100% so I am not sure if the example is valid, but if they asked to shadow inherited method (I can't remember as I've written those tests ages ago) then it's a test-case issue they should address, as it's suggesting bad practices.

kaste commented 4 years ago

If you override, you must call super. That's just an OOP rule. If you trap primitives, you get 1 out of 100000 users which opens an issue why their perfectly fine remove method doesn't get called.

WebReflection commented 4 years ago

Yup, but I think a post/gist explaining common Custom Elements donts could help anyway ...and for this specific use case I'd suggest using drop(...) instead.