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

A draft model instance with an empty ID returns an error when editing #223

Closed Qsppl closed 9 months ago

Qsppl commented 9 months ago

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

image

If the ID is defined, an error occurs when editing the draft image

How to do it right? I tried using false instead of "" as the model id value - didn't work. Tried using -1 and returning an empty model, but that seems like a bad solution.

smalluban commented 9 months ago

It looks like you create not-covered edge cases ;) I made a fix, so just update to v8.2.8 and your case should work.

However, when using the draft mode, I recommend to not use the id setting. When the id is omitted, the model property itself can be set with a model instance or just an id. In your case you share the form with the presentation, which I also recommend to split. So then you have clean model form, which takes a model id, or not if it is a "create" mode:

{
 tag: 'a-comment-form',
 comment: store(Comment, { draft: true }),
 ...
};

Body of a comment component:

   <div>${comment.body}</div>
   ${editMode && html`<a-comment-form comment="${comment}"></a-comment-form>`}

And create form in another place (without setting the comment):

<p>Create comment</p>
<a-comment-form></a-comment-form>
Qsppl commented 9 months ago

Thank you. As for dividing the interface into presentation of models and forms for filling models with data, I wouldn’t want to do this. This reduces the accessibility of actions (in the case of separate edit pages or modal windows), or leads to duplication of markup and logic for "component" and "component-form" (in the case when for editing we replace the component with a form in the same place)

Qsppl commented 9 months ago

Example (changed*): https://codepen.io/qsppl/pen/WNPPdym?editors=1011

Clearing drafts no longer works. It got id "2" and after that I can't set the component property to another draft.

image

Qsppl commented 9 months ago

If creation/editing and presentation really need to be separated into different components, then I’ll work like this. I don't understand very well how hybrids works and maybe I'm trying to do something impossible using one component to create/edit/present a model.

smalluban commented 9 months ago

The problem is only with using the id setting. It then creates (and should) read-only property, so setting draft to null can't work - the property must always take the id from the property set by the id option.

You can change the draft instance by reassigning property, which is used for the id. However, it's impossible to clear draft with id pointing to undefined. You can't have cookie and eat cookie...

So.. I think using id option with draft should not be supported, as it creates that impossible cases. If you omit the id, and set the draft by its own property, everything will work.

Because of the above arguments, I recommend splitting the form from the comment component. But still you can use it inside of the comment, and also use it outside of that context.

Generally, I don't see it as redundant code, rather the opposite - splitting the logic into separate components is usually better.

Qsppl commented 9 months ago

Thanks, I'll do it like this