hybridsjs / hybrids

Extraordinary JavaScript UI framework with unique declarative and functional architecture
https://hybrids.js.org
MIT License
3.03k stars 85 forks source link

version 8.2.17 backwards compatibility is broken! #253

Closed Qsppl closed 4 months ago

Qsppl commented 4 months ago

The change https://github.com/hybridsjs/hybrids/commit/4e111dd2d511b70f7a11d53b0ba0f049a5e9d7c6 breaks backward compatibility even though it is a minor version.

Qsppl commented 4 months ago

I want to get the model and draft for a specific id:

image

This id is determined by the parent <comments-list> via the comment-id attribute.

image

But now it doesn't work.

smalluban commented 4 months ago

You must have been passing props/attrs wrong. This was not a breaking change, just a move of when the same thing happened.

Here you have a working example: https://stackblitz.com/edit/hybrids-simple-counter-mzkstz?file=src%2Findex.html,src%2Findex.js

I suppose, that you pass in your templates something like comment-id="${...}", which is not supported. Please read the docs here: https://hybrids.js.org/#/component-model/templates?id=properties-amp-attributes

Essentially, with that commit attributes are read when an element is upgraded, which is still correct with the docs - you should only use attributes when using them statically in the document/template.

Qsppl commented 4 months ago

https://codepen.io/qsppl/pen/wvZxbOq?editors=1010

I've created an example showing all the invariants of passing properties to a component via attributes in a template. The current behavior is not obvious or intuitive. Many developers on my team also point out that this part of the documentation is difficult to understand.

If passing properties through attributes really should work the way it does now, then could you simplify the documentation on this topic?

smalluban commented 4 months ago

It's actually quite simple, inside of the hybrids templates just always use properties, like modelId, it is just better in every way. I have never gotten confused about it (I know am an author, but I work with other programmers too). I also very rarely use external property to set store model id, as you can just pass it directly to the model name when using factory.

Many developers on my team also point out that this part of the documentation is difficult to understand.

This is something that for example lit-html forces you to explicitly set if you pass a prop or an attribute. And in hybrids, it is here just for interoperability with custom elements created with other framework/vanilajs. You can create a custom element, which watches attribute changes but does not define related property. In that case, and only in that, hybrids template engine will set the attribute rather than the property.

The docs says:

If your template contains a custom element, that only supports attributes, you can use the original name:

I think this is super clear. The template engine can be used with external custom elements, that's why this feature exists.

If passing properties through attributes really should work the way it does now, then could you simplify the documentation on this topic?

You are always welcome to make hybrids better. If you think that docs could be written better, I'll be pleased to review your PR ;)

smalluban commented 4 months ago

One another thought. What could be done, is to add support for case-sensitive attributes inside of templates, but it might be very tricky, as <my-element modelId="1"> is just an element where there is no expression, so no entry point to look at.

In another point of view, this is just a limitation of the HTML itself, as it does not support case-sensitive attributes.

On another hand, the template engine cannot assume that <my-element model-id="${...}"> should set modelId property, as the custom element can just utilize the model-id attribute - and you cannot check it in any way from outside of the element definition.

Qsppl commented 4 months ago

I don't think this is necessary. I will try to more clearly describe under what conditions and how it is necessary to pass property values ​​through attributes - I think this will be enough.

Qsppl commented 4 months ago

But English is not my native language, so the text may have a strange style)

smalluban commented 4 months ago

I thought about the problem, and I added support for fallback to the camel-cased name in an expression: bdd8716951d6ba64fe7b27e78ab769a8c3fa4908

Also docs are updated and simplified.

You can try it out in v8.2.20

Qsppl commented 4 months ago

thank you very much, it became much more convenient