WebReflection / hyperHTML

A Fast & Light Virtual DOM Alternative
ISC License
3.06k stars 112 forks source link

2.34.x is a breaking change, should be 3.0.x #406

Closed vimtaai closed 3 years ago

vimtaai commented 3 years ago

Hi,

We faced an issue when upgrading to 2.34 version of hyperhtml. The new behaviour with the ?disabled=${value} option causes breaking changes when using the null value.

Previous behaviour: disabled=${null} sets the disabled property on the DOM element to false Current behaviour: disabled=${null} sets the disabled property on the DOM element to true

Because of this breaking change in behaviour the major version should be bumped to 3.x.x to indicate the breaking change.

WebReflection commented 3 years ago

wait ... the breaking change wasn't meant, and ?disabled=${value} is opt-in, not mandatory ... are you using ?disabled or just disabled?

WebReflection commented 3 years ago

to clarify, nothing touches the previous behavior here and that's what went out

vimtaai commented 3 years ago

We used it without the ?. Several of our integration tests broke because of the upgrade.

WebReflection commented 3 years ago

From which version?

vimtaai commented 3 years ago

Just a quick check on the code: Line 1501 in index.js has a single = in the if condition. Was this intentional?

WebReflection commented 3 years ago

You don’t reach that code and yes, double (( )) means intentional

WebReflection commented 3 years ago

https://github.com/WebReflection/hyperHTML/commit/ac14025efbf27cffbc776ebe9b8e270d4d71af13#diff-a1e35bf752b708a7f4ff7570d938ff532b7c714aa45ca92dd6f9b5dae70a677aR56

WebReflection commented 3 years ago

Don’t look at anything that is not under esm folder, just to clarify

vimtaai commented 3 years ago

Just trying to make sense of the code, maybe I'm completely off track:

https://github.com/WebReflection/hyperHTML/blob/7a89190551a01a8a58faa1459e28f61d77cc39e2/index.js#L1622-L1628

If I'm correct this is the snippet that runs for using disabled=${null}. In this case shouldn't the function return after removing the attribute on line 1625? It seems to be contradictory to remove an attribute then assigning a new value to it on line 1628.

WebReflection commented 3 years ago

I've tested the code (as there are a lot of tests, we use this in production) and I cannot reproduce the issue ... can you reproduce the issue? Again, nothing changed in here in 2.34, you never reach changed code if there's no ?disabled=${...}

WebReflection commented 3 years ago

also, like I've said, I won't comment on anything that is not under the esm/ folder, as nothing else matters.

WebReflection commented 3 years ago

last, but not least, the attribute value is syncd'

if (attribute.value !== newValue) {

can you help me reproducing this issue? I think you have something else going on there ... I can't reproduce this issue.

vimtaai commented 3 years ago

I'll try to reproduce it.

WebReflection commented 3 years ago

anyway, this is the only change and nothing else changed ... so I am pretty confident you have some other issue, or somebody started passing along different values (not null) somewhere else.

vimtaai commented 3 years ago

We were upgrading from version "2.18.0". Could the issue lie somewhere else? 🤔

WebReflection commented 3 years ago

absolutely, as once again, you never touch new code if you haven't changed templates ... but without anything to reproduce, not even via the latest, only you can find out where it broke, why, or how ...

vimtaai commented 3 years ago

I managed to reproduce the issue. The problem lies with custom elements, their property setters specifically.

https://jsfiddle.net/4cv5eahg/

With version 2.18.0 the value null is received by the setter, in version 2.32.0 an empty string is passed. Adding the ? to the render in line 38 prevents the setter to be called altogether.

WebReflection commented 3 years ago

I read null ... which is expected, it passes whatever value to the setter, and this is not going to change ... have you perhaps changed polyfill for custom elements?

the current behavior in your test is expected from Custom Elements, it looks like it was broken before.

vimtaai commented 3 years ago

If you change the hyperHTML version in the HTML file (I put both versions there, just change the comment) to 2.32.0 then on the console it shows an empty string being recieved by the setter.

WebReflection commented 3 years ago

Just FYI, if you want to trigger the accessor, you can use the never ambiguous .disabled=${value} syntax, and the accessor will receive any value, not just strings.

WebReflection commented 3 years ago

Everything else is explained in the latest update: hyperHTML is ambiguous when no specialized syntax is used in the template, and it works unpredictably if the component is defined before or after the element gets created.

this is a known gotcha of custom elements in general, and cannot be fixed in here.

As 2.34 has nothing to do with this, I’ll close this issue as it looks to me everything works as expected, but you were relying on a previous bug.

WebReflection commented 3 years ago

If you change the hyperHTML version in the HTML file (I put both versions there, just change the comment) to 2.32.0 then on the console it shows an empty string being recieved by the setter.

I've missed this, apologies ... checking out

WebReflection commented 3 years ago

So ... 2.18 coincidentally comes from 2018 https://github.com/WebReflection/hyperHTML/commit/cf6bbed5af5f21cb5200f7a03da8033168933117 ... but we are in 2021 ... although the change that bugs you is from 10th of May 2019 https://github.com/WebReflection/hyperHTML/commit/6262e1d7426c2ac3fefde6f8a3e6ebe8b8086630

that changes in particular deals with legacy, and it feels like you're either not interested in it, or you're late to the update hyperHTML party.

Now ... as previously mentioned, hyperHTML had an ambiguity there, and to opt out from such ambiguity, you have now either ?attr or .attr ... the latter is for accessors.

The .disabled=${...} is exactly what you are looking for, and since this library won't break backward compatibility from 2019 today, or ever, I strongly suggest you switch to that syntax, when you mean to access the element.attribute instead, so that nothing will ever be a surprise.

I guess updating has these costs, but at least here it's clear what you should update.