WebReflection / hyperHTML

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

Bug with Interaction Between WebComponents, HyperHtml, and Input value #357

Closed Rybadour closed 5 years ago

Rybadour commented 5 years ago

Hey,

Our team vends a component library to our clients using WebComponents (polyfilled for IE11, Edge, Firefox, Safari) and were using HyperHtml in some of our components.

We recently got a bug report that in IE11 and Edge our input component would show strings like "_dt: 0.323242" when they are set to empty string or null.

It took some doing but I managed to create a fairly minimal reproduction: https://codepen.io/rybadour/pen/qwQBrJ

And here is a standalone link you can test with in IE11: https://s.codepen.io/rybadour/debug/qwQBrJ/jVkpoyQgnnLA

Note that Codepen doesn't seem to support transpiling template literals for us so I had to manually include a shim. But the original code is something like:

hyperHtml.bind(this)`
   <input type="number" value={this.value}></input>
`;
WebReflection commented 5 years ago

the special ID is used only to setup nodes and remove after.

The input you are showing there is invalid (value{this.value} makes not much sense as is), so unless you have a very simple test case that demonstrate it fails, I'm not sure there's anything I should do.

Whatever bug you are filing, please be sure that no third parts library is involved.

We're using hyperHTML in production on Edge too and for months, so it'd be very surprising this issue is a hyperHTML instead of some other error that throws so that hyperHTML cannot finalize its setup.

TL;DR would this show the same issue?

hyperHTML.bind(document.body)`
<input type="number" value=${null}>`;
Rybadour commented 5 years ago

My example code just had a typo. Our original code contains the "=" and you can see the template in the codepen contains the correct code.

I was not able to reproduce this with HyperHTML alone but it seems very likely to be HyperHtml since doing this with "setAttribute" shows no such issue and your code contains this line:

https://github.com/WebReflection/hyperHTML/blob/master/index.js#L933

Are we using HyperHtml with Webcomponents in the recommended way?

WebReflection commented 5 years ago

Are we using HyperHtml with Webcomponents in the recommended way?

I guess not, 'cause as you wrote:

I was not able to reproduce this with HyperHTML alone

I also use HyperHTMLElement since about ever and this issue never came up.

Are your components interfering with attributes hyperHTML is trying to set?

Rybadour commented 5 years ago

You're own documentation recommends to use WebComponents exactly like we do: https://viperhtml.js.org/hyperhtml/documentation/#components-1

I'll try out the 2 other methods for using WebComponents and report back if they contain this problem or not.

Are your components interfering with attributes hyperHTML is trying to set?

I don't see how, as you'll note in the CodePen I linked I'm basically just running HyperHtml.bind() inside the component. I'm only doing one render, no attributes changing.

WebReflection commented 5 years ago

You're own documentation recommends to use WebComponents

check closely the suggested polyfill thought, which is the one used in production by AFrame, StencilJS, Google AMP, and every other Custom Elements based project.

However ...

I'm basically just running HyperHtml.bind() inside the component. I'm only doing one render, no attributes changing.

I think there is a general misunderstanding of how things work in Custom Elements world.

It is true that IE/Edge shows something that shouldn't, but your demo link made me think you'd expect that if the custom element would have a value, that'd be automatically reflected via this.value, which is not the case, nor what would happen.

Check this counter example, it works in Edge too because the value is a getter, not for any other reasons. https://codepen.io/WebReflection/pen/MRZmMa?editors=0010

So, beside IE/Edge showing something that shouldn't happen, do you understand the issue is in your example, since without defining a getter, value means nothing for you KatInput component?

WebReflection commented 5 years ago

I think this is similar to the other bug, and related to domtagger.

I will keep this open until both are fixed.

WebReflection commented 5 years ago

I believe v2.27.0 should've fixed it, please feel free to re-open this bug if that's not the case, thanks.